-
Notifications
You must be signed in to change notification settings - Fork 146
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
Use symbol for organisation icon #452
Conversation
pom.xml
Outdated
@@ -68,7 +68,7 @@ | |||
<revision>2</revision> | |||
<changelist>999999-SNAPSHOT</changelist> | |||
<gitHubRepo>jenkinsci/${project.artifactId}-plugin</gitHubRepo> | |||
<jenkins.version>2.426.3</jenkins.version> | |||
<jenkins.version>2.454</jenkins.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is newer than the next LTS (2.452.1) which means any updates in this plugin will not work for another ~3 months for the majority of users.
I find this not a good solution (as no new features or fixes would make it to LTS users, without forcing this (and all other plugins) to fork), can you come up with an alternative? or should we just leave this PR for 3 months?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restored the existing icon - it'll use the symbol on newer versions and the old icon on older versions of Jenkins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have been updating various plugins to 2.454 for the same reason. I do not see a problem. If there are features or fixes that are requested by LTS users it is very easy to set up a backport branch and cherry-pick them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should at least be a follow-up PR here that reverts 9c7cd56 so we know to clean up the tech debt; otherwise it will be forgotten forever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is very easy to set up a backport branch and cherry-pick them
As far as I am aware CD can not release stable branches - and as shown here there was no need to actually do this bump to get this feature.
We have been updating various plugins to 2.454 for the same reason
that was your personal choice, and if I had seen it elswhere I would have also left the same comment for the same reasons.
If there are features or fixes that are requested by LTS users
the overwhelming majority of users are not using a weekly with this plugin. (18794 or on or below 2.440.2 vs 2434 on a version between 2.441 - 2.451 so approx. 10%.)
So by pure guesswork and extrapolation I would probably say most of the bugs are from LTS users and given that if someone files a bug they would really want a fix or they would not bother, then that would be backporting every fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CD can not release stable branches
Sure it can. You set up the backport branch with the standard script, file PRs to it (or directly push), and then just click the button in the workflow to release from that branch.
most of the bugs are from LTS users
Most bug reports perhaps. But how many of these actually make it into approved and merged PRs? Very few. Most ongoing changes are initiated by maintainers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Click the button was not what I was thinking for when I CD 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested locally with mvn hpi:run
and mvn hpi:run -Djenkins.version=2.454
LGTM thankyou
Follow up to @mawinter69 changes in core (jenkinsci/jenkins#9127). This PR uses the organisation symbol now that the 'New item' page supports displaying them.
Before
After
Happy to change the icon if others have better suggestions - I based it off of the 'New organisation' button in GitHub:
Proposed changelog entries
Submitter checklist
* Use the
Internal:
prefix if the change has no user-visible impact (API, test frameworks, etc.)