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

Use symbol for organisation icon #452

Merged
merged 3 commits into from
May 1, 2024
Merged

Conversation

janfaracik
Copy link
Contributor

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
image

After
image

Happy to change the icon if others have better suggestions - I based it off of the 'New organisation' button in GitHub:

Screenshot 2024-05-01 at 12 56 50

Proposed changelog entries

  • Use symbol for organisation icon

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change).
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests

@janfaracik janfaracik requested a review from a team as a code owner May 1, 2024 11:57
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>
Copy link
Member

@jtnord jtnord May 1, 2024

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

@jtnord jtnord May 1, 2024

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 😂

@jtnord jtnord requested a review from a team May 1, 2024 13:24
Copy link
Member

@jtnord jtnord left a 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

@jtnord jtnord enabled auto-merge May 1, 2024 14:35
@jtnord jtnord merged commit af810c5 into jenkinsci:master May 1, 2024
13 of 14 checks passed
@janfaracik janfaracik deleted the use-symbol branch May 1, 2024 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants