Skip to content

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

@MartinKosicky MartinKosicky requested a review from jglick August 5, 2022 06:56
@jglick jglick requested review from car-roll and dwnusbaum August 5, 2022 14:04
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
@jglick jglick requested a review from MarkEWaite August 5, 2022 15:14
Fixed signature

Co-authored-by: Jesse Glick <jglick@cloudbees.com>
@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.

… exception if caller uses builder directly

Co-authored-by: Devin Nusbaum <dwnusbaum@users.noreply.github.com>
@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