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

[JENKINS-66984] Pick up newer Guava from core #244

Merged
merged 5 commits into from
Nov 15, 2021

Conversation

jglick
Copy link
Member

@jglick jglick commented Oct 26, 2021

JENKINS-66984

Downstream of jenkinsci/jenkins#5707.

#215 and #226 might need to wait for a Guice upgrade.

See e.g. #140 for motivation.

@jglick jglick requested review from Vlatombe and basil October 27, 2021 12:25
@jglick jglick requested a review from jtnord October 27, 2021 12:25
Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

Thanks Jesse

</dependencies>
<dependencyManagement>
<dependencies>
<dependency>
<groupId>io.jenkins.tools.bom</groupId>
<artifactId>bom-2.249.x</artifactId>
<artifactId>bom-2.249.x</artifactId> <!-- TODO weekly -->
Copy link
Member

Choose a reason for hiding this comment

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

would it not be better to use 2.303 so at least you are closed to the core and pick up any other plugins that requrie the new Guava?

Copy link
Member Author

Choose a reason for hiding this comment

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

jenkinsci/bom#671 would be more appropriate for this purpose. I punted on it because #242 (comment) suggests that I would need to make a bigger patch.

@@ -201,7 +201,7 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
wc.getPage(b, "artifact/3/4/");
int httpCount = httpLogging.getRecords().size();
System.err.println("total count: " + httpCount);
assertThat(httpCount, lessThanOrEqualTo(11));
assertThat(httpCount, lessThanOrEqualTo(13));
Copy link
Member

Choose a reason for hiding this comment

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

why does changing the core, change this number? any clues would be helpful in an inline comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Took me some bisecting, but see 19e3b48

@jglick jglick marked this pull request as ready for review November 9, 2021 13:50
@jglick jglick requested a review from jtnord November 9, 2021 13:51
@jglick
Copy link
Member Author

jglick commented Nov 15, 2021

wait for a Guice upgrade

jenkinsci/jenkins#5858 FTR

@jglick jglick merged commit 728e7db into jenkinsci:master Nov 15, 2021
@jglick jglick deleted the guava-JENKINS-66984 branch November 15, 2021 16:56
@car-roll
Copy link

@jglick can we get a new release on this? PCT fails on NetworkTest without these changes

@jglick
Copy link
Member Author

jglick commented Nov 17, 2021

OK. It looks like jenkinsci/jenkins#5858 has been released, too, so I can try taking advantage of that next.

@jglick
Copy link
Member Author

jglick commented Nov 17, 2021

https://github.com/jenkinsci/artifact-manager-s3-plugin/releases/tag/artifact-manager-s3-1.17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants