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

Reduce guava footprint #991

Merged
merged 8 commits into from May 19, 2021
Merged

Reduce guava footprint #991

merged 8 commits into from May 19, 2021

Conversation

car-roll
Copy link
Contributor

@car-roll car-roll commented May 14, 2021 •
edited

Reduce the use of Guava from the plugin in order to be compatible with all future Jenkins versions where Guava may be bumped or not exposed.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

Vlatombe
Vlatombe reviewed May 14, 2021
View reviewed changes
src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesProvisioningLimits.java
@@ -121,13 +120,11 @@ public void unregister(@Nonnull KubernetesCloud cloud, @Nonnull PodTemplate podT
}

@Nonnull
@VisibleForTesting
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep something like // VisibleForTesting ? This has still documentation value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wondering if there was an alternative java annotation or access modifier we can use?

Copy link
Member

Choose a reason for hiding this comment

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

Well, we could maybe use something like @Restricted(NoExternalUse.class)

@car-roll
Copy link
Contributor Author

@Vlatombe do you know if the kind test failures are valid or if it is just a CI issue? i'm having issues installing/running microk8s so I can't tell.

@car-roll car-roll marked this pull request as ready for review May 18, 2021 18:49
@Vlatombe
Copy link
Member

@car-roll they should be passing in general. There can be some flakes due to infrastructure.
I checked the jackson failures (ContainerExecDecoratorTest) in this pr and they're not in master, so this seems like a regression.

@car-roll car-roll closed this May 19, 2021
@car-roll car-roll reopened this May 19, 2021
@car-roll car-roll requested a review from Vlatombe May 19, 2021 16:33
@Vlatombe Vlatombe added the enhancement Improvements label May 19, 2021
@Vlatombe Vlatombe merged commit f4aa445 into jenkinsci:master May 19, 2021
@car-roll
Copy link
Contributor Author

car-roll commented May 19, 2021 •
edited

@Vlatombe thanks! Just a heads up, I don't think release drafter picked up this PR in the release notes?
edit: lol, nevermind, i misread the link in the release. i see this pr is in the next one.

@Vlatombe
Copy link
Member

Don't worry, it did.

@MRamonLeon
Copy link

@Vlatombe could you release it please? Or do you know when it may be released? thanks!

@Vlatombe
Copy link
Member

@MRamonLeon done (1.29.6)

This was referenced Aug 7, 2021
Open
Closed
Closed
@Vlatombe Vlatombe mentioned this pull request Nov 10, 2021
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vlatombe Vlatombe Vlatombe approved these changes

Assignees
No one assigned
Labels
enhancement Improvements
Projects
None yet
Milestone
No milestone
3 participants
@car-roll @Vlatombe @MRamonLeon