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-42971] New API for SCMFileSystem to allow for lightweight checkout with build params #160

Merged
merged 8 commits into from
Aug 9, 2022

Conversation

MartinKosicky
Copy link
Contributor

@MartinKosicky MartinKosicky commented Jul 28, 2022

https://issues.jenkins.io/browse/JENKINS-42971

I added a new Builder class into the SCMFileSystem so that we have backward compatibility with current SCM implementations and with ability for a new implementation, so that they can make for example branch substitutions based on parameters in the build (environment).

@MartinKosicky
Copy link
Contributor Author

I would fix the blockers ASAP but first I would like some feedback if this concept does make sence, or if you would prefer a different approach. I dont have problems doing it, but I dont want to do major fixes if I am not sure I am on the right track :)

@MartinKosicky MartinKosicky marked this pull request as draft July 29, 2022 06:43
@MartinKosicky MartinKosicky changed the title New API for SCMFileSystem to allow for lightweight checkout with build params [JENKINS-42971] New API for SCMFileSystem to allow for lightweight checkout with build params Jul 31, 2022
@MartinKosicky MartinKosicky marked this pull request as ready for review July 31, 2022 16:14
@MartinKosicky
Copy link
Contributor Author

MartinKosicky commented Jul 31, 2022

I checked it working by updating workflow-cps plugin and done some local changes to git plugin, that can be only added for PR if this PR will be succesful

@jglick
Copy link
Member

jglick commented Aug 1, 2022

that can be only added for PR if this PR will be successful

FYI once you have a passing build of a PR up to date with its base branch, you will get an “Incrementals” check (here, https://repo.jenkins-ci.org/incrementals/org/jenkins-ci/plugins/scm-api/619.vb_7498d1edb_18/) and it is possible to refer to that version in the <dependencyManagement> section of a POM in a downstream plugin (git or workflow-cps, say). Typically we would open draft PRs downstream like

<dependency>
  <groupId>…</groupId>
  <artifactId>…</artifactId>
  <version>123.vdeadbeef9999</version> <!-- TODO https://github.com/jenkinsci/upstream-plugin/pull/123 -->
</dependency>

representing the link and permitting CI to run a full test suite in advance.

@jglick
Copy link
Member

jglick commented Aug 1, 2022

Did you look at #78?

@MartinKosicky
Copy link
Contributor Author

src/main/java/jenkins/scm/api/SCMFileSystem.java Outdated Show resolved Hide resolved
src/main/java/jenkins/scm/api/SCMFileSystem.java Outdated Show resolved Hide resolved
src/main/java/jenkins/scm/api/SCMFileSystem.java Outdated Show resolved Hide resolved
src/main/java/jenkins/scm/api/SCMFileSystem.java Outdated Show resolved Hide resolved
src/main/java/jenkins/scm/api/SCMFileSystem.java Outdated Show resolved Hide resolved
@jglick jglick requested a review from MarkEWaite August 5, 2022 15:14
@MartinKosicky
Copy link
Contributor Author

MartinKosicky commented Aug 8, 2022

Hello @jglick , I would just like to ask if the current flow is that multiple reviewers have to approve or if there are any improvements I can do here regarding this scm-api :)

@jglick
Copy link
Member

jglick commented Aug 8, 2022

if the current flow is that multiple reviewers have to approve

Not in general, though for significant changes and especially API additions (which cannot generally be reverted if they turn out to be misconceived) I prefer to have a second opinion.

@MartinKosicky
Copy link
Contributor Author

if the current flow is that multiple reviewers have to approve

Not in general, though for significant changes and especially API additions (which cannot generally be reverted if they turn out to be misconceived) I prefer to have a second opinion.

Yes that sounds reasonable. I will wait :)

Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

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

The new APIs seem fine to me.

src/main/java/jenkins/scm/api/SCMFileSystem.java Outdated Show resolved Hide resolved
src/main/java/jenkins/scm/api/SCMFileSystem.java Outdated Show resolved Hide resolved
… exception if caller uses builder directly

Co-authored-by: Devin Nusbaum <[email protected]>
@car-roll car-roll merged commit daab055 into jenkinsci:master Aug 9, 2022
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.

4 participants