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

Removed usage of Guava #603

Merged
merged 1 commit into from
Apr 29, 2021
Merged

Removed usage of Guava #603

merged 1 commit into from
Apr 29, 2021

Conversation

rsandell
Copy link
Member

@rsandell rsandell commented Apr 27, 2021

To secure a future upgrade in core.

  • 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

To secure a future upgrade in core.
@@ -129,7 +131,7 @@ public void sendInput(byte[] input) {

public boolean slurpOutput(FastPipedOutputStream stdout, FastPipedOutputStream stderr) throws IOException {
LOGGER.log(Level.FINE, () -> "--> SlurpOutput");
ImmutableMap<String, FastPipedOutputStream> streams = ImmutableMap.of("stdout", stdout, "stderr", stderr);
Map<String, FastPipedOutputStream> streams = new HashMap<>(); streams.put("stdout", stdout); streams.put("stderr", stderr);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not an immutable but I think it's fine. We only use it internally. Maybe you can initialize to 2 elements, as they're the only elements we will use

@@ -21,7 +20,7 @@ public OpenShellRequest(URL url) {

protected void construct() {
try {
defaultHeader().action(new URI("http://schemas.xmlsoap.org/ws/2004/09/transfer/Create")).resourceURI(new URI("http://schemas.microsoft.com/wbem/wsman/1/windows/shell/cmd")).options(ImmutableList.of(new Option("WINRS_NOPROFILE", "FALSE"), new Option("WINRS_CODEPAGE", "437")));
defaultHeader().action(new URI("http://schemas.xmlsoap.org/ws/2004/09/transfer/Create")).resourceURI(new URI("http://schemas.microsoft.com/wbem/wsman/1/windows/shell/cmd")).options(Arrays.asList(new Option("WINRS_NOPROFILE", "FALSE"), new Option("WINRS_CODEPAGE", "437")));
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not immutable as before but we don't use the list afterwards anyway.


Header(String to, String replyTo, String maxEnvelopeSize, String timeout, String locale, String id, String action, String shellId, String resourceURI, ImmutableList<Option> optionSet) {
Header(String to, String replyTo, String maxEnvelopeSize, String timeout, String locale, String id, String action, String shellId, String resourceURI, List<Option> optionSet) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Better API

@@ -27,7 +29,7 @@
this.action = action;
this.shellId = shellId;
this.resourceURI = resourceURI;
this.optionSet = optionSet;
this.optionSet = optionSet != null ? Collections.unmodifiableList(new ArrayList<>(optionSet)) : Collections.emptyList();
Copy link
Contributor

Choose a reason for hiding this comment

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

👏🏽

Copy link
Contributor

@res0nance res0nance left a comment

Choose a reason for hiding this comment

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

I agree with Ramon that dropping immutable in all these cases do not matter, so LGTM

@res0nance res0nance merged commit 0a66458 into jenkinsci:master Apr 29, 2021
@rsandell rsandell deleted the no-guava branch May 4, 2021 08:42
@rsandell
Copy link
Member Author

rsandell commented May 4, 2021

@res0nance Thanks for merging!
When can we get a release?

@res0nance
Copy link
Contributor

@res0nance Thanks for merging!
When can we get a release?

Hey @rsandell, I'm good to release whenever but #602 seems to have received a comment, might be worth checking it out.

I don't think there is too much urgency around Guava removal but yes we definitely want users to start adopting this version before core(hopefully) manages to remove it.

@alecharp
Copy link
Member

alecharp commented May 4, 2021

@res0nance the comment on #602 is ok, you can go ahead and release the plugin.

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

Successfully merging this pull request may close these issues.

4 participants