Refactoring for new major version

strangelookingnerd commented 6 months ago

Over the last days I began refactoring the code base, aiming to provide a "fresh start" for this plugin and introducing a new major version.

Fixes https://issues.jenkins.io/browse/JENKINS-73167 and #149 alongside #148 and likely many other regressions introduced in the last version through #130 and other changes that we have not uncovered yet.

Changes

See https://github.com/jenkinsci/badge-plugin/pull/151#issuecomment-2122929246

Start new major version 2.0

Mark plugin to be incompatible with previous version

`addBadge` reworked

Refrain from building CSS styles or classes in jelly but rather have them as user input
All text can be plain text or HTML (with Safe HTML MarkupFormatter enabled)
Can be used for both, text and icon badges
Supports built-in icon, Jenkins icons and symbols as well as icons and symbols of other plugins
New fields:
- id - optional identifier of the badge
- text - text used for the badge or the hover text when an icon is selected
- icon - icon used for the badge
- cssClass - optional CSS class to be applied to a badge
- style - optional CSS style to be applied to a badge
- link - optional link for a badge

`addInfoBadge` , `addWarningBadge`, `addErrorBadge` reworked

Inherit directly from addBadge

New `addSummary`

Substitutes createSummary and has all feature the other badges have
Allows having a link in the summary

Manipulate a badge during build time

All pipeline steps now return the action that was created and allow to manipulate a badge in a pipeline

def badge = addBadge(icon: 'everything-is-awesome.png', text: 'All is good!')
...
badge.setText('Somehing bad happened!')
badge.setIcon('everything-is-on-fire.png')

HTML formatting delegated to global Jenkins MarkupFormatter

Breaking: Remove Badge Plugin global configuration
All text is formatted using Jenkins.get().getMarkupFormatter().translate()
- Breaking: It is no longer possible to use non-sanitized HTML in badges (unless there is a MarkupFormatter for it, that I'm unaware of)
- This is a huge security improvement that prevents any malicious injections via HTML / JavaScript and makes configuration easier

Deprecations

Deprecate addHtmlBadge (can be substituted by addBadge)
- Remains backwards compatibility for existing pipelines and configs
- Add deprecation notices to build output
Deprecate addShortText (can be substituted by addBadge)
- Remains backwards compatibility for existing pipelines and configs
- Add deprecation notices to build output
Deprecate createSummary (can be substituted by addSummary)
- Remains backwards compatibility for existing pipelines and configs
- Add deprecation notices to build output

Testing

Added more tests resulting in > 90% coverage

Documentation

Update documentation to reflect current functionallity.
Since some of these changes are rather big we give users a guidance for migration (still in progress)
Remove all documentation of deprecated badges

@jenkinsci/badge-plugin-developers Please let me know what you think, happy to get some feedback.

### Submitter checklist
- [x] Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main branch!
- [ ] Ensure that the pull request title represents the desired changelog entry
- [x] Please describe what you did
- [x] Link to relevant issues in GitHub or Jira
- [x] Link to relevant pull requests, esp. upstream and downstream changes
- [x] Ensure you have provided tests - that demonstrates feature works or fixes the issue

MarkEWaite commented 6 months ago

The main points so far:

Allow all text to be plain or HTML (and apply safe HTML formatting).

I like that idea

Remove addHtmlBadge as it does not provide any value over addBadge (given that any text is treated as HTML)

That will break compatibility with existing Pipeline scripts. Jenkins plugins should not break compatibility with existing Pipelines unless there is a very strong reason to do so. I'd very much prefer that we retain addHtmlBadge even if HTML is allowed in the text strings.

strangelookingnerd commented 6 months ago

Remove addHtmlBadge as it does not provide any value over addBadge (given that any text is treated as HTML)

That will break compatibility with existing Pipeline scripts. Jenkins plugins should not break compatibility with existing Pipelines unless there is a very strong reason to do so. I'd very much prefer that we retain addHtmlBadge even if HTML is allowed in the text strings.

I totally understand that but am wondering: How could I deprecate / remove pipeline steps provided in a graceful manner? What about parameters that need to change?

Another point that came up was changing the groupId to io.jenkins.plugins as well as switching the package names to io.jenkins.plugins. The latter will likely cause incompatibilities for existing badges / summaries as their references will no longer work. Is there a graceful way to mitigate that?

MarkEWaite commented 6 months ago

I totally understand that but am wondering: How could I deprecate / remove pipeline steps provided in a graceful manner? What about parameters that need to change?

As far as I can tell, we can't remove Pipeline steps without breaking users and Jenkins users depend on not being broken by changes from plugin maintainers.

Another point that came up was changing the groupId to io.jenkins.plugins as well as switching the package names to io.jenkins.plugins. The latter will likely cause incompatibilities for existing badges / summaries as their references will no longer work. Is there a graceful way to mitigate that?

Changing the groupId will create a new plugin. Users generally won't know that a new plugin was created. The existing plugin will no longer receive updates and the new plugin will only be adopted by users if they become aware of its existence. Changing groupId or artifactId of a Jenkins plugin is a bad thing for users. Daniel Beck provides more detail in a reply to a Jenkins developers mailing list post.

strangelookingnerd commented 6 months ago

Alright, I will try to work around any breaking changes and see what I end up with. I think starting a new plugin is not really what I was aiming for 👍🏼 Stay tuned for more changes to come in the following days.

One thing I wanted to take a closer look at is the MarkupFormatter that is right now configured solely for this plugin. I think it would actually be better and more importantly a security improvement to rely on the "global" formatter configured for Jenkins via https://github.com/jenkinsci/jenkins/blob/5089ad5e5962d22294d48d57e5cccc68b26c5236/core/src/main/java/jenkins/model/Jenkins.java#L1766 Another topic would be error handling for some of the steps / actions. There are some instances where a faulty parameter may cause the pipeline to abort, I don't like that too much if I'm honest as there should always be a proper fallback. Further I saw that createSummary does allow manipulating the summary, I'd love to implement the same for badges as well.

mPokornyETM commented 6 months ago

The main points so far:

Allow all text to be plain or HTML (and apply safe HTML formatting).

I like that idea

Remove addHtmlBadge as it does not provide any value over addBadge (given that any text is treated as HTML)

That will break compatibility with existing Pipeline scripts. Jenkins plugins should not break compatibility with existing Pipelines unless there is a very strong reason to do so. I'd very much prefer that we retain addHtmlBadge even if HTML is allowed in the text strings.

@strangelookingnerd it is much more simple, then you think. You can provide warning, in the function (step) now. Like this step si deprecated and will be removed end of September ... . Or something like that. You can also add new jenkins warnings for Jeniins adminsistrator like "Java 11 end of life in Jenkins" does now. That means, the jenkins administrators, have "enought" time to provide changes.

but general I 100% agree with @MarkEWaite . It is not good to deprecate some pipeline step. I as Jenkins admin in my company will be verry unhappy, when we need to update Jenkins ASAP (because of some security issue fix) and the I need to spend many hours to fix things, like deprecated functions.

The minimum is in that case "Breaking change" description in the PR.

strangelookingnerd commented 6 months ago

I just pushed the latest changes that are slowly going towards a final state. I did my very best to keep backwards compatibility for existing pipeline scripts as well as already persisted build.xml for existing builds. All the legacy functions that are no longer supported (but still functional!) are marked as deprecated and should not need to be touched. Therefor I also removed all their documentation as I do not feel the need to promote outdated functionallty. Further I added logging outputs to be build console in case there is a deprecated pipeline step being used in order to warn users about a future removal / migration path. This adds some not-so-pretty code which I don't like too much, but it is what it is.

In order to actually validate the compatibility I used the latest state (db43b1b54761b721802c66aa91ecfb4ed58b8eb0) to create test jobs for every documentated pipeline step configuration. I guess that should cover everything that may or may not be working right and show how my new approach handles it. Once the jobs where created and run I took screenshots. Then I took my current branch and used the exact same jobs to find out what may have changed. This is mostly visually and some things like mouseovers are missing in the screenshot but be assured I validated them manually.

Here is the comparision using sliders, so it's easier to see what changed. Please notice that everyghing is zoomed in by 150%:

no.	comparison	comment
1	https://imgsli.com/MjY1OTE5/0/1	slightly different alignment
2	https://imgsli.com/MjY1OTE5/2/3	slightly different alignment
3	https://imgsli.com/MjY1OTE5/4/5	slightly different alignment
4	https://imgsli.com/MjY1OTE5/6/7	slightly different alignment
5	https://imgsli.com/MjY1OTE5/8/9	identical
6	https://imgsli.com/MjY1OTE5/10/11	identical
7	https://imgsli.com/MjY1OTE5/12/13	slightly different alignment
8	https://imgsli.com/MjY1OTE5/14/15	slightly different alignment
9	https://imgsli.com/MjY1OTE5/16/17	slightly different alignment, ~~border gets removed~~ (border is restored)
10	https://imgsli.com/MjY1OTE5/18/19	slightly different alignment,
11	https://imgsli.com/MjY1OTE5/20/21	slightly different alignment
12	https://imgsli.com/MjY1OTE5/22/23	slightly different alignment
13	https://imgsli.com/MjY1OTE5/24/25	identical
14	https://imgsli.com/MjY1OTE5/26/27	identical

As for the alignments, I suspect that it is because of the <span> I added to enclose the badges. I needed something to attach the style and class attributes to when I do not have a link. Right now I do not see a way around it. However it is barely noticeable and should not be a concern and a slight change in behavior barely anyone will ever notice. The only real difference can be seen in no. 9 - the addShortText(text) does no longer have a default border around its text. I would argue that this is actually a bug in the existing implementation as I can't imagine it being intentional. Even though this is a noticeable change in behavior I would strongly argue that it is actually an improvement.

Please let me know what you think and I will in the meantime tackle the test coverage 👍🏼 If you need my job configuration to validate this yourself, please ping me.

mPokornyETM commented 6 months ago

You shall resove you merge conflicts

mPokornyETM commented 6 months ago

@strangelookingnerd it is really hard to review all the changes. pls resolve your conflicts, afterwards, I can download the plugin and try it on my test-instance. A full review is more or less not possible. Sorry

strangelookingnerd commented 6 months ago

The conflicts have been resolved.

strangelookingnerd commented 6 months ago

The PR is ready for review. I updated the description to reflect a high level view of the changes that are contained. The only thing that is currently missing is the migration guide which I plan to make part of the release notes.

Excited to hear what @jenkinsci/badge-plugin-developers think about this.

mawinter69 commented 6 months ago

It would be good to validate the changes against https://github.com/jenkinsci/jenkins/pull/9148 which does a complete rewrite of the history widget

strangelookingnerd commented 6 months ago

It would be good to validate the changes against jenkinsci/jenkins#9148 which does a complete rewrite of the history widget

@mawinter69 That's a great idea! Thanks for bringing it up.

I'm just checking the current state against the incremental (2.460-rc35013.a_441f2fd8c5c). It looks promising, the misalignments noted in https://github.com/jenkinsci/badge-plugin/pull/151#issuecomment-2122929246 seem to be gone.

The only thing I find a little odd is the build status icon now being 20x20 instead of 16x16 (class icon-sm) - is this intentional?

Things start to get a little messy once you have many badges:

Once there are many badges they start to break the surrounding div - one could argue that adding this many badges is not a good idea to begin with but still should be looked into.

strangelookingnerd commented 5 months ago

@jenkinsci/badge-plugin-developers

I would love to get this merged soon-ish especially since https://github.com/jenkinsci/jenkins/pull/9148 will make a visual change to the build history that I would love to use as a baseline for an upcoming 2.0 release of the badge-plugin.

I know the change set is quite huge however I would feel better if another pair of eyes would take a look at these changes and approve them.

MarkEWaite commented 5 months ago

I know the change set is quite huge however I would feel better if another pair of eyes would take a look at these changes and approve them.

I'm afraid that I won't be able to review it for several months. The Spring Security 6.x Upgrade project needs to have most of my attention for the next several months.

Please be mindful of the importance of compatibility to Jenkins users. Compatibility is so important to Jenkins users that it is included in the Jenkins governance documents where it says:

We recognize that users expect their existing data, accumulated under past versions (including Hudson up to 1.395) to continue working under future versions of Jenkins. This includes jobs configurations, build records, and plugin binaries that they are using. The Jenkins project places high value on maintaining this compatibility, and will be very careful in removing functionality.

Retaining compatibility is hard work but it matters deeply to Jenkins users.

strangelookingnerd commented 5 months ago

I'm afraid that I won't be able to review it for several months. The Spring Security 6.x Upgrade project needs to have most of my attention for the next several months.

...

Retaining compatibility is hard work but it matters deeply to Jenkins users.

I see, maybe some of the other maintainers can step up and give it a shot.

As for the compatibility, I'm very confident that my changes will not cause issues for existing pipelines or build configurations. There are some minor visual changes (see https://github.com/jenkinsci/badge-plugin/pull/151#issuecomment-2122929246) that - in my opinion - should be considered improvements that do not have any negative impact. I put in great effort to ensure the compatibility with automated as well as manual tests, using the current stable version as well as the future 2.0 version in combination with different Jenkins versions and possible scenarios. Still it would be good to have another pair of eyes to take a look at my changes.

mPokornyETM commented 5 months ago

@strangelookingnerd did you seen my comments from the last review?

strangelookingnerd commented 5 months ago

@strangelookingnerd did you seen my comments from the last review?

Not sure what you are referring to. Can you link me the comments?

mPokornyETM commented 5 months ago

@strangelookingnerd did you seen my comments from the last review?

Not sure what you are referring to. Can you link me the comments?

@strangelookingnerd I marked you in all of them

strangelookingnerd commented 5 months ago

@strangelookingnerd I marked you in all of them

If you are referring to https://github.com/jenkinsci/badge-plugin/pull/151#issuecomment-2121854951:

I have recently updated the PR description to reflect the current / final state of my changes. There are no breaking changes but only deprecations. All deprecated functionality remains intact and working as before. There are also warning outputs when using deprecated pipeline steps.

mPokornyETM commented 3 months ago

@strangelookingnerd I marked you in all of them

If you are referring to #151 (comment):

I have recently updated the PR description to reflect the current / final state of my changes. There are no breaking changes but only deprecations. All deprecated functionality remains intact and working as before. There are also warning outputs when using deprecated pipeline steps.

@strangelookingnerd do you see this comments ?

strangelookingnerd commented 3 months ago

@strangelookingnerd I marked you in all of them

If you are referring to #151 (comment): I have recently updated the PR description to reflect the current / final state of my changes. There are no breaking changes but only deprecations. All deprecated functionality remains intact and working as before. There are also warning outputs when using deprecated pipeline steps.

@strangelookingnerd do you see this comments ?

I actually don’t, sorry. Maybe you need to finish your review by approving/requesting changes for them to become public?

strangelookingnerd commented 2 months ago

Merging the changes and creating v1.x branch in case we need it. I will start working on a changelog and cut a release hopefully end of next week.

jenkinsci / badge-plugin