-
Notifications
You must be signed in to change notification settings - Fork 190
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-33273] Allow lightweight checkouts to be used from CpsScmFlowDefinition #97
Conversation
pom.xml
Outdated
@@ -199,7 +199,7 @@ | |||
<dependency> | |||
<groupId>org.jenkins-ci.plugins</groupId> | |||
<artifactId>git</artifactId> | |||
<version>2.4.1</version> | |||
<version>3.0.2-beta-2</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will pick up jenkinsci/git-plugin#467 when available.
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
Queue.Executable _build = owner.getExecutable(); | ||
if (!(_build instanceof Run)) { | ||
throw new IOException("can only check out SCM into a Run"); | ||
} | ||
Run<?,?> build = (Run<?,?>) _build; | ||
if (lightweight) { | ||
SCMFileSystem fs = SCMFileSystem.of(build.getParent(), scm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use try with resources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 forgetting to close SCMFileSystem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐝
@@ -0,0 +1,5 @@ | |||
<div> | |||
If selected, try to obtain the Pipeline script contents directly from the SCM without performing a full checkout. | |||
The advantage of this mode is its efficiency; however, you will not get any changelogs or polling based on the SCM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to get the changelogs with the SCMFilesystem
API...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would require a significant rewrite. Currently workflow-job
knows to record changelogs, or consider polling, based on SCMListener
methods which cannot be called in lightweight mode. I considered such a large change (APIs etc.) out of scope here, especially given that
- many people will not care about changelogs or polling for the SCM containing the script
- if you
checkout scm
the same repo later, you will get those anyway
@@ -29,6 +29,9 @@ THE SOFTWARE. | |||
<f:entry field="scriptPath" title="${%Script Path}"> | |||
<f:textbox default="Jenkinsfile"/> | |||
</f:entry> | |||
<f:entry field="lightweight" title="${%Lightweight checkout}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think you would at most want an advanced option to not try lightweight. If you can do lightweight, why wouldn't you want to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I preferred to err on the side of leaving existing behavior alone by default for now.
- lack of changelog/polling support (see below)
- new code path with likely bugs compared to original
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This box is selected by default. I think it is safer to maintain existing behavior for existing Pipelines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is what we are doing—default
is used for new definitions.
pom.xml
Outdated
@@ -95,7 +95,7 @@ | |||
<dependency> | |||
<groupId>org.jenkins-ci.plugins</groupId> | |||
<artifactId>scm-api</artifactId> | |||
<version>1.1</version> | |||
<version>2.0.1</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should ideally be bumped up to whatever release actually makes it to the UC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐝 but I am concerned about excessive options being added
…t lack of support for changelogs, polling, and build parameters.
This looks mergable? |
Needs a code review, and would like signoff from @HRMPW as well. |
The UI change is minimal and looks good. 🐝 |
[JENKINS-57085] Avoid complex StackTraceElement.fileName values
Analogue of jenkinsci/workflow-multibranch-plugin#47 for standalone Pipeline projects.
@reviewbybees