Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serve jquery-3.1.1.min.js locally #2378

Merged
merged 1 commit into from
Aug 10, 2020
Merged

Serve jquery-3.1.1.min.js locally #2378

merged 1 commit into from
Aug 10, 2020

Conversation

chaseSpace
Copy link
Contributor

@chaseSpace chaseSpace commented Aug 10, 2020

Which problem is this PR solving?

Short description of the changes

If some Country or Area cannot access https://code.jquery.com/jquery-3.1.1.min.js, e.g. China; the example hotrod is also cannot be running normally, please see my problem desc from #2375 .

Solving

  • download jquery-3.1.1.min.js from https://code.jquery.com/jquery-3.1.1.min.js
  • move it to examples\hotrod\services\frontend\web_assets\jquery-3.1.1.min.js
  • change to ./jquery-3.1.1.min.js from https://code.jquery.com/jquery-3.1.1.min.js in html file examples\hotrod\services\frontend\web_assets\index.html
  • regenerate examples\hotrod\services\frontend\gen_assets.go

Testing

The major change is a html file and a auto-generated go file; There are no changes to go files that involve important logic in this PR; So no new unit-test code, but I've tested it locally.

@chaseSpace chaseSpace requested a review from a team as a code owner August 10, 2020 04:05
@@ -3,7 +3,7 @@
<meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=0">
<head>
<title>HotROD - Rides On Demand</title>
<script src="https://code.jquery.com/jquery-3.1.1.min.js"></script>
<script src="./jquery-3.1.1.min.js"></script>
<link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css" integrity="sha384-BVYiiSIFeK1dGmJRAkycuHAHRg32OmUcww7on3RYdg4Va+PmSTsz/K68vbdEjh4u" crossorigin="anonymous">
<script src="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/js/bootstrap.min.js" integrity="sha384-Tc5IQib027qvyjSMfHjOMaLkfuWVxZxUPnCJA7l2mCWNIpG9mGCD8wGNIcPD7Txa" crossorigin="anonymous"></script>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about the Bootstrap CSS and JS? Wouldn't they present similar issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested for load two bootstrap files locally; It will have some impact on the page display; moreover, this address can be accessed in China, so I don't think it is necessary to modify the loading of these two files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, works for me

@yurishkuro
Copy link
Member

please make sure your commits are signed

@chaseSpace
Copy link
Contributor Author

chaseSpace commented Aug 10, 2020

please make sure your commits are signed

I just signed it, do I need restart workflow test?

@codecov
Copy link

codecov bot commented Aug 10, 2020

Codecov Report

Merging #2378 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2378   +/-   ##
=======================================
  Coverage   95.60%   95.61%           
=======================================
  Files         206      206           
  Lines       10548    10548           
=======================================
+ Hits        10084    10085    +1     
+ Misses        396      394    -2     
- Partials       68       69    +1     
Impacted Files Coverage Δ
cmd/query/app/server.go 90.90% <0.00%> (-2.28%) ⬇️
plugin/storage/integration/integration.go 80.86% <0.00%> (+0.47%) ⬆️
...lugin/sampling/strategystore/adaptive/processor.go 100.00% <0.00%> (+0.79%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4275e0c...e7adb5f. Read the comment docs.

@yurishkuro
Copy link
Member

You can see the DCO check that does not pass. Your signing email must match your GitHub account's email (and possibly name).

@chaseSpace
Copy link
Contributor Author

DCO passed.

@chaseSpace
Copy link
Contributor Author

I don't know why ci failed, is it related to my modification?

@chaseSpace
Copy link
Contributor Author

Could you help me find out why I failed in the test?

@yurishkuro yurishkuro changed the title serve jquery-3.1.1.min.js locally Serve jquery-3.1.1.min.js locally Aug 10, 2020
@yurishkuro yurishkuro merged commit bbc90c1 into jaegertracing:master Aug 10, 2020
@yurishkuro
Copy link
Member

Thanks!

yurishkuro pushed a commit that referenced this pull request Aug 10, 2020
The version included in #2378 was somehow different from what's available via https://code.jquery.com/jquery-3.1.1.min.js

Signed-off-by: Yuri Shkuro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

example [hotrod] can't work properly on my centos
2 participants