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

Removing code to support the Remoting-based CLI #3838

Merged
merged 13 commits into from Feb 15, 2019
Merged

Conversation

@jglick
Copy link
Member

jglick commented Jan 9, 2019

As proposed here. Follows up #2795 by removing the code which that change deprecated. Also reverts the JDK-8-specific test module introduced in #3759, as these tests are no longer needed leaves the JDK 8 test module but deletes all the ysoserial tests from it.

If accepted, these docs would need some updates as well.

@jenkinsci/code-reviewers

Proposed changelog entries

  • The -remoting option to the Jenkins CLI has been removed. Commands and command options which required this mode have been removed from Jenkins core and will no longer work in plugins; the login, logout, install-tool, set-build-parameter, and set-build-result commands have been removed. The -p and --username/--password options are also removed. Some public classes and methods which were already deprecated as specific to Remoting mode have been removed, though those likely to be used by plugins have been retained for compatibility.
jglick added 2 commits Jan 9, 2019
@jglick jglick requested review from oleg-nenashev and jeffret-b Jan 9, 2019
@jtnord

This comment has been minimized.

Copy link
Member

jtnord commented Jan 9, 2019

@mikecirioli as disturbed can you check our internal CLI for any issues with this?

…roblem was concealed by the original code not checking the command exit code.
@jglick jglick requested a review from daniel-beck Jan 9, 2019
@jvz
jvz approved these changes Jan 9, 2019
Copy link
Member

jvz left a comment

I love deleting old code.

image

Copy link
Contributor

jeffret-b left a comment

It looks good to me. As I said over on the mailing list I'm in favor of removing this deprecated, problematic capability.

I've looked over the changes and they make sense but I haven't done a full in-depth analysis of each change. The build passes locally and on CI so that's a good indicator.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Jan 11, 2019

The latest test failure appears to be due to broken UC JSON.

@batmat batmat closed this Jan 12, 2019
@batmat batmat reopened this Jan 12, 2019
@Wadeck
Wadeck approved these changes Jan 14, 2019
Copy link
Contributor

Wadeck left a comment

🐝 Seems a good idea!

@batmat batmat self-requested a review Jan 14, 2019
@Wadeck Wadeck mentioned this pull request Jan 25, 2019
4 of 4 tasks complete
Copy link
Member

oleg-nenashev left a comment

I am -1 w.r.t this change, see the developer mailing list. Other maintainers are welcome to ignore my -1 and merge the pull request taking votes

test/pom.xml Outdated Show resolved Hide resolved
@batmat

This comment has been minimized.

Copy link
Member

batmat commented Feb 1, 2019

For reference, Oleg's email on the ML is https://groups.google.com/forum/#!msg/jenkinsci-dev/upgi9c5u63o/W6UmnDwUDwAJ

Copying my answer here, because it applies to this PR context well too:


I understand your point Oleg, but I feel quite strongly that we shouldn't keep the new test-jdk8 module empty or so for a "just in case" reason.

  1. In case we ever need it again, we can very easily then revert the removal using Git, and reintroduce this module when needed.
  2. in the meantime, it would be deeply misleading for everyone, and mainly newcomers that there is an empty maven module in the source tree.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Feb 1, 2019

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Feb 1, 2019

the entire structure which enables creating Java-version-specific tests

It seemed like a fairly straightforward change in either direction: moving a couple of POMs around and adding a <profile>.

And there will be likely merge conflicts anyway.

Right, I could split off the test-jdk8 refactoring into a separate commit if that would address your concerns but due to Git behavior as noted in #3838 (comment) it is unlikely that a future git revert --no-commit on that would be any easier than just looking at #3759 and doing something similar, especially since you would anyway want to delete the actual current JDK 8-only tests!

@batmat

This comment has been minimized.

Copy link
Member

batmat commented Feb 1, 2019

IIUC this is great: we have a path forward don't we?

Maybe we could simply:

  • Have a new PR, or repurpose this one, that would still do the remoting CLI removal, but only empty the test-jdk8 module from its code instead of removing it
    • IIUC again, as Oleg does not disagree on the CLI removal, he would approve that (?)
  • Then we possibly file a separate PR (or rather first meet to find an agreement) to do the test-jdk8 module removal part

WDYT?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Feb 1, 2019

IIUC again, as Oleg does not disagree on the CLI removal, he would approve that (?)

I would. It just needs to be highlighted in the changelog, because there are still users out there.

but only empty the test-jdk8 module from its code instead of removing it

I am fine with it either. If @jglick is concerned about the empty module, I am happy to provide another Java 8-only test case

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Feb 4, 2019

I am happy to provide another Java 8-only test case

You mean, find a retroactive justification?

Anyway, sounds like we have a way of making progress, so I can amend this accordingly, and #3872 can adapt.

It just needs to be highlighted in the changelog

Absolutely! I included proposed text in the PR description, but I would presume that whoever does the weekly changelog editing would want to boldface parts of this or something.

…t working there for some reason).
Copy link
Contributor

jeffret-b left a comment

I took a look through it again. It still looks good. This is a valuable cleanup.

jglick added 2 commits Feb 6, 2019
Copy link
Member

oleg-nenashev left a comment

LGTM. Assuming it is properly communicated.

@jglick would you be up to write a short blogpost announcing the removal of the functionality and the CLI commands? I doubt we can offer mitigation for that, but it may be possible to offer somebody some hints to revive the functionality as a standalone plugin if they are ready to invest time into it

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Feb 6, 2019

@daniel-beck As a Security Officer, are you fine with removing Class deserialization tests?

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Feb 6, 2019

would you be up to write a short blogpost announcing the removal of the functionality and the CLI commands?

Yes this makes sense, let me think about it.

hints to revive the functionality as a standalone plugin

I doubt that is feasible, much less desirable, and would not offer any hope of it.

@batmat

This comment has been minimized.

Copy link
Member

batmat commented Feb 7, 2019

docker run --rm -ti -p 8080:8080 -p 50000:50000 -e ID=3838 jenkins/core-pr-tester for anyone to test this quite easily.

Though now we have Incrementals, it might even be simpler to use https://repo.jenkins-ci.org/incrementals/org/jenkins-ci/main/jenkins-war/2.164-rc27949.9c80cf12deb0/jenkins-war-2.164-rc27949.9c80cf12deb0.war

jglick added 2 commits Feb 12, 2019
@oleg-nenashev oleg-nenashev removed the on-hold label Feb 14, 2019
@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Feb 14, 2019

Taking the agreements above, it is no longer blocked
@jglick please feel to proceed with fixing CI and merging

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Feb 14, 2019

The test failures were due to an update center outage, but I will start a new build just to be sure.

@jglick jglick merged commit cc964ef into jenkinsci:master Feb 15, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/incrementals Deployed to Incrementals.
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
@jglick jglick deleted the jglick:remotingCLI branch Feb 15, 2019
@Wadeck

This comment has been minimized.

Copy link
Contributor

Wadeck commented Feb 15, 2019

🎉

@batmat batmat mentioned this pull request Apr 11, 2019
batmat added a commit to batmat/evergreen-plugin that referenced this pull request Apr 26, 2019
This stub accodomates the removal of the Remoting CLI in
jenkinsci/jenkins#3838

The change does two things:
* reintroduce the symbol by providing an no-op object for it
* have some code to rewrite the JCasC yaml files without this option
jglick added a commit to jglick/jenkins that referenced this pull request Dec 17, 2019
…er parsing CLICommand arguments.
@jglick jglick mentioned this pull request Dec 20, 2019
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.