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-67725 - Extending BranchBuildStrategies to allow setting last revision built + access to SCMEvent #291

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

Conversation

carpnick
Copy link

@carpnick carpnick commented Feb 3, 2022

See JENKINS-67725 for more context.

  • 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
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

The basic use case:

  • How do we enable the folks to "save" the first commit ID found, and only build everything after that. This update allows us to implement a BranchBuildStrategy to do that. Without this change we are not handed the right pieces to implement that use case.

Discussion

  • As written this will always evaluate to true even if a BuildStrategy keeps responding, do not build. The next time a branch indexing comes in, it will evaluate to true since it did not build. This feels like a design flaw. As a plugin author I am telling you to not build it. As a Jenkins Admin I am telling you not to build it. But you are repeatedly trying to build it if I turn on branch indexing.
  • As I see it we have 2 options
    • We extend BranchBuildStrategy and add another optional method that allows a plugin author to own if you can update the last built hash
    • We create a whole new extension point to give a plugin author the ability to define what changesDetected means.

What is in the PR

  • Added a new extension method(optional) for plugin authors can choose to update last built revision or not. Default is false which is current behavior
  • Added integration of new method into MultiBranchProject to evaluate if it should update lastbuiltrevision on disk if told not to build.
  • Also added SCMEvent to the extension methods. Allows plugin authors to use properties to determine if we should build the artifact. The specific property I am looking for is timestamp. If I create a plugin that requires only building forward(from now) I need to know when the event was generated. This gives me that ability.

Please review at your convenience. This is ready.

@carpnick carpnick changed the title JENKINS-67725 - Extending BranchBuildStrategies to allow setting last revision built JENKINS-67725 - Extending BranchBuildStrategies to allow setting last revision built + access to SCMEvent Feb 3, 2022
@carpnick
Copy link
Author

Could I get a review @jtnord and/or @rsandell ?

@carpnick
Copy link
Author

carpnick commented Mar 1, 2022

Any kind of review would be appreciated...
@timja or @car-roll

@timja timja requested a review from a team March 1, 2022 13:41
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.

a very quick first pass check seems like this has the potential to break things.

I would also expect at least one downstream PR to demonstrate this new API.

ref: https://www.jenkins.io/doc/developer/extensions/branch-api/#branchbuildstrategy

I need to find some time to go through this in some more detail, but I do not have that time right now,

@CheckForNull SCMRevision lastBuiltRevision,
@CheckForNull SCMRevision lastSeenRevision,
@NonNull TaskListener listener){
throw new UnsupportedOperationException("Modern implementation accessed using legacy API method");
Copy link
Member

Choose a reason for hiding this comment

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

this seems like a breaking change? if an Extension overrides this calss and updated to the modern way removing the over ride on this deprecated function - then any consumers that have not updated will not blow up at runtime?

If the extension can do something about it - then it would seem this method is just un-needed - or could be used to call the modern method using the
Util.isOverridden trick.

Copy link
Author

Choose a reason for hiding this comment

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

I actually followed the example of what the rest of the plugin extension did. IE - Line 111 was originally shipped with 2.0.17 while line 138 was shipped with 2.1.3. Basically everytime there is a new version, the previous version is set to UnsupportedOperationException.

So the way I understand it the SPI method finds out dynamically which method is overridden (original, V2, V3, etc) and then calls that specific override. So the SPI method uses Util.isOverridden to determine which API method to call.

src/main/java/jenkins/branch/MultiBranchProject.java Outdated Show resolved Hide resolved
@jtnord jtnord requested a review from a team March 1, 2022 13:56
Co-authored-by: James Nord <jtnord@users.noreply.github.com>
@carpnick
Copy link
Author

carpnick commented Mar 7, 2022

I would also expect at least one downstream PR to demonstrate this new API.

ref: https://www.jenkins.io/doc/developer/extensions/branch-api/#branchbuildstrategy

Fair comment - I will either do a PR in one of the plugins that exists and/or create a plugin and link it here.

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