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

Add a trait to expose PathRestriction extension #573

Closed
wants to merge 1 commit into from

Conversation

@atikhono
Copy link

atikhono commented Feb 14, 2018

Hi, I've implemented the PathRestriction trait as I cannot migrate my build-flow projects to multibranch without this feature (polling that ignores certain paths in repository). This patch is not a merge-ready solution. It's an intermediate result that resolved a particular problem with some limited settings. Being and unexperienced programmer I'm really looking forward to an advice which would lead to the implementation of the feature that can be a part of a future release.

I have created a multibranch project and committed my Jenkinsfiles with different properties(PipelineTrigger) setting in each Jenkinsfile. I've configured the multibranch project scanner to run periodically (every 2 minutes), and set "suppress automatic SCM triggering" for all branches because that's how I want them to behave. I want branch builds to start by SCM polling only and at their own schedule, which is configured by PipelineTrigger. With this setup, polling worked as desired; branch builds started with the configured schedule and only when there were new commits on their tops.

After I added the PathRestrictionTrait class, multibranch polling stopped working properly. It kept running with the schedule, but the branch builds started regardless of new commits on that branches. It happens because PathRestriction requires a workspace for polling, so the execution takes another path in GitSCM.compareRemoteRevisionWith. Multibranch uses another BuildChooser -- SpecificRevisionBuildChooser.AbstractGitSCMSource -- and an own implementation of getCandidateRevisions, which has no support for polling calls. I've sketched a minimal working solution that is limited by DEFAULT_REMOTE_NAME (couldn't see an easy way to get remote name from a trait in this context) and doesn't update the BuildChooser revision. Could you please advice me on an implementation that could be accepted? @MarkEWaite @stephenc @reviewbybees

Copy link
Member

oleg-nenashev left a comment

typos and the copyright need to be fixed. The getCandidateRevisions code looks good, but I am not familiar with this code to say for sure. Extra tests would be nice

/*
* The MIT License
*
* Copyright (c) 2018 CloudBees, Inc.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Feb 14, 2018

Member

Please use a personal copyright, it is not a CloudBees code

*/
@Override
public String getDisplayName() {
return "Polling ingnores commits in certin paths";

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Feb 14, 2018

Member

There are some typos here. Please also use Messages.properties so that the string is localizable


/**
* Exposes {@link PathRestriction} as a {@link SCMSourceTrait}.
*

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Feb 14, 2018

Member

Add @since TODO

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Feb 15, 2018

So a few points:

  1. We really want to avoid adding new UI visible things to the git plugin, changes to the git plugin going forward will focus on enabling extension plugins to be written that add the UI visible things. This means that this PR shall not be merged as long as it is adding a trait. An extension plugin is trivial to write as all you need is the right pom.xml and move your trait to that plugin. (If, after the move, you require additional changes in the git plugin in order to let you write an extension plugin then those changes are perfectly fine for this PR)

  2. I am leaving this PR open as to help provide code review easily until you are ready to move the code to an extension plugin.

  3. This is IMHO a case of “you can’t get there from here”. I think what you actually want is a BranchBuildStrategy. That is able to decide if the branch should be automatically built based on the current revision and the previous revision. You could then use SCMFileSystem to get the changes between the two and by walking the changes see if all changed paths are ignored. Bonus I think such an extension can be SCM agnostic and of utility to all. No workspace would be required for GitHub or Bitbucket but the repo cache would be populated for other git extension SCMSource. This also means that all branches could share the repo cache.

@atikhono atikhono force-pushed the atikhono:path-restriction branch from cc6dc3c to 16b0e79 Feb 19, 2018
@atikhono

This comment has been minimized.

Copy link
Author

atikhono commented Feb 19, 2018

@stephenc I've removed the trait from the PR and prepared an extension plugin with that trait in it: https://github.com/atikhono/git-path-restriction-extension-plugin. BranchBuildStrategyis about Multibranch project scanner, isn't it? That means, builds from all branches start at the time the project is scanned. Builds from all branches start at the same time. I want to avoid that behaviour; that I can do with polling. Another way to go for me might be to limit the number of concurrent builds in the whole Multibranch project, which hasn't been implemented yet AFAIK.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Feb 28, 2018

@oleg-nenashev oleg-nenashev requested a review from MarkEWaite Feb 28, 2018
@jglick

This comment has been minimized.

Copy link
Member

jglick commented Feb 28, 2018

This longstanding RFE is discussed in JENKINS-35988. Since it is a very common requirement, effectively a functional regression compared to non-multibranch projects, it would make sense to me for the trait to be defined in scm-api, with a standard BranchBuildStrategy in branch-api; means there would need to be a cache repository-based implementation in git and something using the GitHub API in github-branch-source. Needs design/API work “blessed” by experienced contributors to these plugins.

@atikhono

This comment has been minimized.

Copy link
Author

atikhono commented Mar 21, 2018

@jglick Thank you very much! I do understand your input, but. Is Multibranch pipeline somehow applicable to projects that cannot be built per-commit? E.g big projects in active development that take more than an hour to build on limited hardware. As far as I can see, implementing PathRestriction extension the way you described would result in all updated branches running at the time Multibranch project scanning runs as it is the only thing that triggers them (please correct me if I'm wrong). With polling I could configure a schedule the way I do it for FreeStyle and Pipeline jobs so that I can avoid flooding the hardware with long-running per-commit builds. Probably it's reasonable to keep support polling for such projects, isn't it? The draft implementation I have done in this PR could be rewritten to use SCMFileSystem instead of a workspace for polling. However, right now I cannot assess if it is a big change or not. Does it make any sense? @stephenc

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Mar 21, 2018

In my opinion, you can ignore all @jglick’s suggestions about adding things in scm-api

The needed api in scm-api is the SCMFileSystem api and that should provide the required functionality to get the change log between two revisions and hence see the affected paths.

A BranchBuildStrategy extension plugin can thus look at the changes revisions and decide if the change should be built or not on the basis of the affected paths reported by SCMFileSystem. This should be completely SCM agnostic (though a SCMSource that hasn’t implemented the changelog support will be unable to use path excluding)

The build strategy will be consulted both for each event received through the event hook and during indexing but in both cases only if the last built revision is not equal to the current revision.

If you want to slow down some branches from being built too often, you can use a build throttle property on those branches.

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Mar 21, 2018

Note: I am not saying we don’t need to improve scm-api, rather there should be enough to get something working... once we see the usage patterns and the hot spots, then we can optimise and cache those issues... but getting something working first is better than premature optimisation ;-)

@MarkEWaite MarkEWaite added this to the Later milestone Dec 11, 2018
@MarkEWaite MarkEWaite closed this Dec 11, 2018
@MarkEWaite MarkEWaite added the bugfix label Jul 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.