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.

Sign up for GitHub

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

Jump to bottom

Omit WEB-INF/lib/jquery-detached-1.2.jar from jenkins.war #4120

Merged
merged 1 commit into from Jul 24, 2019
Merged

Omit WEB-INF/lib/jquery-detached-1.2.jar from jenkins.war #4120

merged 1 commit into from Jul 24, 2019

Conversation

jglick
Copy link
Member

@jglick jglick commented Jul 18, 2019

This file is a nuisance since it is a plugin JAR and so can confuse PCT runs: workflow-cps depends on 1.2.1 and will not be loaded when the tested plugin has this JAR in the test classpath. (#3215 would also fix the symptom, since the improperly bundled version would at least be newer.)

As far as I know this happens only in PCT when target/test-classes/test-dependencies/jquery-detached.hpi is 1.2.

Tested java -jar style and all the JS I could think of worked as expected:

  • setup wizard
  • ACE editor in Pipeline
  • stage view

@jglick jglick mentioned this pull request Jul 18, 2019
Merged
raul-arabaolaza
raul-arabaolaza approved these changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@raul-arabaolaza raul-arabaolaza left a comment

Choose a reason for hiding this comment

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

I can not think in more concrete tests to be run... But I believe if this breaks seriously the UI the ath tests should catch that... And anyway only the boostrap plugins use jquery not in general so aproving

oleg-nenashev
oleg-nenashev approved these changes Jul 21, 2019
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Looks fine. Should not we also make it a detached plugin just to be on the safe side? Id has 200k_ installations so should be fine

@oleg-nenashev oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label Jul 21, 2019
@oleg-nenashev oleg-nenashev requested a review from a team July 21, 2019 08:59
Wadeck
Wadeck approved these changes Jul 22, 2019
View reviewed changes
Copy link
Contributor

@Wadeck Wadeck left a comment •
edited

Choose a reason for hiding this comment

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

Due to https://github.com/jenkinsci/jenkins/pull/4120/files#diff-82c9d27714c878724d651c3c4a016ddbR138, that should not have a negative impact. (= we remove the 1.2 indirect dependency to keep only the direct 1.2.1)

Not tested manually.

As sse-gateway-plugin uses jQuery but did not declare it itself (rely on core I suppose), it could be interesting to also test it, WDYT? Jesse was totally right, only used in test and sample, nothing to worry about here.

@oleg-nenashev oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Jul 22, 2019
@oleg-nenashev
Copy link
Member

Will merge it tomorrow if no negative feedback

@jglick
Copy link
Member Author

jglick commented Jul 22, 2019

As sse-gateway-plugin uses jQuery but did not declare it itself (rely on core I suppose), it could be interesting to also test it, WDYT?

I have no idea how one would test functionality of that plugin. Anyway jQuery seems to be used only from a sample and from a load tester, not from the plugin itself.

@jglick
Copy link
Member Author

jglick commented Jul 22, 2019

Should not we also make it a detached plugin just to be on the safe side?

jQuery is used by core so it cannot be a plugin.

@oleg-nenashev
Copy link
Member

@Wadeck are you fine with the responses?

@Wadeck
Copy link
Contributor

Wadeck commented Jul 24, 2019

@oleg-nenashev Yeah I modified my comment just after Jesse's reply :)

@oleg-nenashev oleg-nenashev merged commit 01e85d6 into jenkinsci:master Jul 24, 2019
@jglick jglick deleted the jquery-detached branch July 24, 2019 10:20
jglick added a commit to jglick/bom that referenced this pull request Oct 4, 2019
@jglick
Adjusting PCT exclusions:
1c09193 
· working around jenkinsci/branch-api-plugin#167
· jenkinsci/jenkins#4120 is in 2.190.x
· so is jenkinsci/jenkins#4099
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raul-arabaolaza raul-arabaolaza raul-arabaolaza approved these changes

@daniel-beck daniel-beck daniel-beck approved these changes

@Wadeck Wadeck Wadeck approved these changes

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

@tfennelly tfennelly Awaiting requested review from tfennelly

Assignees
No one assigned
Labels
internal ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Milestone
No milestone
5 participants
@jglick @oleg-nenashev @Wadeck @raul-arabaolaza @daniel-beck