[JENKINS-31086] Add useDefaultExcludes option to stash #266
Conversation
|
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. |
|
Closing/reopening - those failures look spurious to me. |
|
Can't reproduce this locally - had one test failure on |
|
Rebased. |
|
Woo, and now the tests actually decided to pass. =) |
| " }\n" + | ||
| " writeFile file: 'at-top', text: 'ignored'\n" + | ||
| " stash name: 'from-top', includes: 'elsewhere/'\n" + | ||
| " semaphore 'ending'\n" + |
There was a problem hiding this comment.
🐜 These three lines serve no apparent function in this test case and could have been skipped along with the asserts about StashManager.
There was a problem hiding this comment.
Which three lines? Not sure what you're referring to.
There was a problem hiding this comment.
writeFile file: 'at-top', text: 'ignored'
stash name: 'from-top', includes: 'elsewhere/'
semaphore 'ending'all copied from smokes but not testing anything novel here. The only thing this test case needs to check is that subdir/.gitignore was stashed.
There was a problem hiding this comment.
I'd like to keep the stash - I like having the test first stash without default excludes and then do a second stash with default excludes enabled to make sure it's covering both options.
There was a problem hiding this comment.
But the second stash is not even including subdir, so it is not testing the implicit useDefaultExcludes: true.
There was a problem hiding this comment.
Yeah, it is - the unstash is within dir('elsewhere'), and the original stash is done from within subdir, so we're not actually ever stashing the directory, just its contents.
There was a problem hiding this comment.
You could, for example, do something like
dir('subdir') {stash 'stripped'}
// …
unstash 'stripped'
echo "have .gitignore? ${fileExists '.gitignore'}"and assert that it prints
have .gitignore? false
There was a problem hiding this comment.
Ah, ok. That is simpler. Reworking it.
|
Addressed comments, re-pushed. |
| " stash name: 'from-top', includes: 'elsewhere/'\n" + | ||
| "}", true)); | ||
| WorkflowRun b = p.scheduleBuild2(0).waitForStart(); | ||
| assertEquals("{from-top={elsewhere/other=more}, whatever={.gitignore=whatever, other=more}}", StashManager.stashesOf(b).toString()); |
There was a problem hiding this comment.
🐛 this is not going to work reliably. The build might have completed (deleting the stash) by the time you hit this line.
My suggestion was to delete both the calls to StashManager and SemaphoreStep, not just one.
There was a problem hiding this comment.
Yeah, now I get where you were going and have a new version about to get pushed.
There was a problem hiding this comment.
FYI, to get this to work, I did need to create a second file so that the stash with implicit useDefaultExcludes doesn't error out due to no files to stash.
|
Yet more addressing and pushing! |
| import org.jvnet.hudson.test.BuildWatcher; | ||
| import org.jvnet.hudson.test.Issue; | ||
| import org.jvnet.hudson.test.JenkinsRule; | ||
|
|
||
| import static org.junit.Assert.assertEquals; |
There was a problem hiding this comment.
FWIW usual code style is to use static imports for these.
There was a problem hiding this comment.
Sorry, yes, static wildcard imports.
|
🐝 |
|
Hopefully one last round of addressing comments. =) |
|
fyi, https://jenkins.ci.cloudbees.com/job/plugins/job/workflow-plugin/1955/ is the relevant build for the current state. |
|
🐝 |
|
Fixed comment. |
|
🐝 |
Defaults to true, meaning use Ant default excludes for DirScanner.Glob. If false, don't use the default excludes, to pick up files like .gitignore etc.
|
Rebased, merging. |
[JENKINS-31086] Add useDefaultExcludes option to stash
JENKINS-31086
Defaults to true, meaning use Ant default excludes for
DirScanner.Glob. If false, don't use the default excludes, to pick up
files like .gitignore etc.
@reviewbybees