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

[FIX JENKINS-40750] Remove cc.xml from core #2691

Merged
merged 2 commits into from
Apr 9, 2019

Conversation

daniel-beck
Copy link
Member

@daniel-beck daniel-beck commented Jan 2, 2017

As this doesn't involve data or other compatibility concerns, I think we can skip the bundled/detached plugins dance in this case.

To be merged only once ccxml plugin exists. WIP plugin: https://github.com/jenkinsci/cctray-xml-plugin

Not sure how to handle toCCStatus, between keep/keep and deprecate/keep, deprecate, and throw exception/remove I opted for the second (after review) in this PR as it appears to be unused everywhere. Feedback welcome.

Proposed changelog entries

* Major RFE: Remove built-in support for CCtray (cc.xml) files. To restore this feature, install the <a href="https://plugins.jenkins.io/cctray-xml">CCTray XML Plugin</a>

@daniel-beck daniel-beck added needs-more-reviews Complex change, which would benefit from more eyes on-hold This pull request depends on another event/release, and it cannot be merged right now and removed on-hold This pull request depends on another event/release, and it cannot be merged right now labels Jan 2, 2017
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.

The binary compatibility issue needs to be fixed. I was also surprised there is no existing Cruise Control plugin for Jenkins...

@daniel-beck
Copy link
Member Author

@oleg-nenashev Addressed your review comment.

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.

The current state is fine for me if it is properly changelogged and then mentioned in the upgrade guide for the next LTS

@oleg-nenashev
Copy link
Member

@daniel-beck Ridiculously, there was a mention of such integration at one of Russian IT websites (link, in Russian). CC @SvyatoslavRazmyslov. They use CCTray. So maybe this feature as not that dead as we expect. But we can still decouple it to a plugin.

@oleg-nenashev
Copy link
Member

UPD: English version of the post is here

@daniel-beck daniel-beck added on-hold This pull request depends on another event/release, and it cannot be merged right now 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 Feb 20, 2017
@daniel-beck
Copy link
Member Author

@oleg-nenashev Right, I never expected this to be unused -- just no reason to keep it in core, and questionable popularity and no data storage combined mean I don't see the need to add it to the DETACHED_PLUGINS.

@oleg-nenashev
Copy link
Member

@daniel-beck since the LTS line is picked, maybe it's a good time to ship it.

/me grumbles about the the API deprecation policy, which would not be easily able to handle such cases anyway

@batmat batmat added the unresolved-merge-conflict There is a merge conflict with the target branch. label Dec 28, 2018
@batmat
Copy link
Member

batmat commented Dec 28, 2018

@daniel-beck this is now conflicted, but apparently this was (almost) ready to merge? IIUC, this is still missing a release from https://github.com/jenkinsci/cctray-xml-plugin/ ?

@batmat batmat removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jan 4, 2019
@oleg-nenashev
Copy link
Member

@daniel-beck are you still interested in this PR?

@oleg-nenashev oleg-nenashev added the stalled The PR is reasonable and might be merged, but it is no longer active. It can be taken over by other label Jan 28, 2019
@daniel-beck
Copy link
Member Author

@oleg-nenashev I am. Now's probably a good time to implement this given the LTS cycle.

@daniel-beck daniel-beck removed stalled The PR is reasonable and might be merged, but it is no longer active. It can be taken over by other unresolved-merge-conflict There is a merge conflict with the target branch. labels Apr 4, 2019
@batmat
Copy link
Member

batmat commented Apr 5, 2019

I don't use it personally, and never did. I even had to dig into various websites to understand what this feature was.

But still:

As this doesn't involve data or other compatibility concerns, I think we can skip the bundled/detached plugins dance in this case.

Makes me slightly uncomfortable. That's fine-ish to me, but I have no idea how much this feature is used. But generally we'd add it to split-plugins.txt and the likes. Doing it would be quite easy and guarantee compatibility.

Copy link
Member

@batmat batmat left a comment

Choose a reason for hiding this comment

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

Given it is said to be deprecated and desupported on various websites after a quick search. Given I had no idea what this was before digging into it, I am going to selfishly consider I suppose not a majority of people still know and use CCTray.

So, it looks like extracting this feature as a plugin that users will have to explictly install looks fine.
Let's just be sure we surface it clearly both in the weekly changelog and the LTS upgrade guide, and everything should go mostly fine I think.

@daniel-beck
Copy link
Member Author

I also prepared https://github.com/jenkinsci/cctray-xml-plugin for release. It currently depends on an incrementals deployment of this PR, so everyone can try how it would work.

@daniel-beck daniel-beck removed the on-hold This pull request depends on another event/release, and it cannot be merged right now label Apr 5, 2019
@daniel-beck daniel-beck removed their assignment Apr 5, 2019
@batmat
Copy link
Member

batmat commented Apr 5, 2019

I filed jenkinsci/cctray-xml-plugin#3 so Incrementals can actually be published.

UPDATE: OK, I read it better and understood seeing the current plugin failure. The plugin is using a Core Incremental version.
Fix upcoming for the linked PR: DONE

@batmat batmat added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Apr 9, 2019
@batmat
Copy link
Member

batmat commented Apr 9, 2019

Just marked as ready for merge given the approvals. I'm just waiting for last confirmation from @daniel-beck that he's ready to release the plugin.
We'll also want a clear changelog entry for end users in next weekly, and then next LTS.

@daniel-beck
Copy link
Member Author

I'm ready. The plugin has been updated and currently has a dependency on an incremental on core, once this is out I'll release it and reference it in the changelog (and possibly release a temporary 0.1 a bit early with a lower core dependency to make sure it's on the update center).

@batmat
Copy link
Member

batmat commented Apr 9, 2019

OK, let's pull the trigger then. Thanks everyone.

@batmat batmat merged commit 5db814b into jenkinsci:master Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants