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

[JENKINS-74864] Default library step changelog to global configuration settings #100

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Dohbedoh
Copy link

@Dohbedoh Dohbedoh commented Dec 18, 2023

JENKINS-74864

Testing done

  • Stand up a controller
  • Configure a "Global Pipeline Library"
    • Uncheck "Include @Library changes in job recent changes"
    • Ensure "Load implicitly" is not checked
  • Create two Pipelines
    • First pipeline uses the @Library annotation to load the above library and use a test step
    • Second pipeline uses the library step to do the same

Defaults

  • Run the pipelines
  • Make a new commit to the shared library repository
  • Run the pipelines again

--> The second run of both pipelines shows NO changes

Global configuration is the default

  • Reconfigure the global pipeline library
    • Check "Include @Library changes in job recent changes"
  • Make a new commit to the shared library repository
  • Run the pipelines again

--> The new run of both pipelines shows changes

Library step overrides / takes precedence

  • Reconfigure the pipeline with the library step:
    • Add the option changelog: false
  • Make a new commit to the shared library repository
  • Run the pipelines again

--> The new run of the pipeline with the annoation shows changes
--> The new run of the pipeline with the library step shows NO changes

Submitter checklist

Preview Give feedback

@Dohbedoh Dohbedoh requested a review from a team as a code owner December 18, 2023 12:47
@Dohbedoh
Copy link
Author

Dohbedoh commented Dec 18, 2023

Need some advice for that last test failure - the object changelog attribute does not match the descriptor jelly https://github.com/jenkinsci/pipeline-groovy-lib-plugin/blob/master/src/main/resources/org/jenkinsci/plugins/workflow/libs/LibraryStep/config.jelly#L32 anymore. Should we change the UI descriptor default ? Or adapt the test ?

@jglick
Copy link
Member

jglick commented Dec 18, 2023

From my PoV this is as designed: the changelog configuration is intended to apply to use of the library from either the @Library annotation or implicitly (for libraries loaded by default). If you are writing the library step then it is up to you to pick whether you want changelogs to be included or not. If there even is a global or folder-scoped library corresponding to this step (no retriever) then it would be nice for form validation in the snippet generator to warn you if you are picking the opposite value from that definition, and/or the default value in config.jelly could be picked up from the definition. Changing the constructor value would switch the runtime behavior of the step for pipelines which do not specify the parameter.

@Dohbedoh
Copy link
Author

Dohbedoh commented Dec 20, 2023

When seeing the issue reported by the user, I also assumed that this was a bug. As a user, it feels logical that if you don't set the changelog explicitly it somehow takes the default from the global configuration. Especially when other configurations (version, cache, ...) are inferred from the global configuration of the library. From a dev perspective, seeing the use of at defaultedChangesets at https://github.com/jenkinsci/pipeline-groovy-lib-plugin/blob/689.veec561a_dee13/src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryStep.java#L184 also makes me think it was the intent.

It might explain the confusion that we see in several threads:

But I see you point. It's going to change change the behavior of existing pipelines that use the library step, and will require a breaking change disclaimer, though maybe it is for the best ?

@jglick
Copy link
Member

jglick commented Dec 20, 2023

other configurations (version, cache, ...) are inferred from the global configuration

Perhaps, I do not recall this detail. Discussion in jenkinsci/workflow-cps-global-lib-plugin#34 (comment) was six years ago.

will require a breaking change disclaimer, though maybe it is for the best

I do not think an incompatible change is warranted for a possibly minor unexpected behavior with what should be an obvious “workaround” (or simply “solution”, depending on your perspective): specifying the value of the step parameter to be what you wanted.

@jglick
Copy link
Member

jglick commented Dec 20, 2023

the confusion that we see in several threads

Some of those are discussing Declarative, which may be adding its own (possibly buggy or inconsistent) behavior on top of the library step itself.

@Dohbedoh Dohbedoh changed the title [JENKINS-72440] Default library step changelog to global configuration settings [JENKINS-74864] Default library step changelog to global configuration settings Nov 15, 2024
@Dohbedoh
Copy link
Author

(recreate the JIRA. I was removed I recall because there was an JIRA DB outage at that time).

@jglick
Copy link
Member

jglick commented Nov 15, 2024

(merge conflicts ICYMI)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants