upgrade workflow-api to list (un)stashed files#133
upgrade workflow-api to list (un)stashed files#133joseblas wants to merge 2 commits intojenkinsci:masterfrom
Conversation
| <revision>2.23</revision> | ||
| <changelist>-SNAPSHOT</changelist> | ||
| <jenkins.version>2.249.1</jenkins.version> | ||
| <jenkins.version>2.264-SNAPSHOT</jenkins.version> |
There was a problem hiding this comment.
Use 2.263, and please file a separate PR for removing reflection, so that is not all mixed in here.
There was a problem hiding this comment.
ok about removing reflection, but some changes will needed in Jenkins-core (jenkinsci/jenkins#5023 (review)) so a new version will be needed.
| private @CheckForNull | ||
| String includes; | ||
| private @CheckForNull | ||
| String excludes; |
There was a problem hiding this comment.
Please avoid gratuitous whitespace changes.
| String gibberish = file.getCanonicalPath().split("/" + name + "/")[0]; | ||
| logger.println("stashed file " + StringUtils.substring(file.getCanonicalPath(), gibberish.length())); | ||
| } catch (IOException | InterruptedException e) { | ||
| logger.println("stashed file " + file.getName()); |
| } | ||
|
|
||
| @DataBoundSetter | ||
| public void setVerbose(boolean verbose) { |
There was a problem hiding this comment.
Does this really need to be an explicit option? If printing messages is useful, just do it by default, and maybe truncate display after ten or twenty lines so you do not get too spammy.
There was a problem hiding this comment.
indeed, this is a good example why printing should be done in here, if the list is truncated or not is responsibility of this plugin, not workflow-api which has to supply that list only.
There was a problem hiding this comment.
If you just get rid of the step option and print some reasonable level of detail by default then you need no API change and no patch to this plugin (other than perhaps an integration test).
There was a problem hiding this comment.
I'd rather do the right thing, which is (to me)to pass the files (un)stashed back to the step to print it if appropriate.
There was a problem hiding this comment.
The verbose parameter needs to be optional as the task describes, and by default should be false to ensure that the behavior is consistent with the previous releases. So, this does not need to be trunked.
There was a problem hiding this comment.
Why do we care if the behavior is identical to previous releases? This is just diagnostic logging as a convenience.
There was a problem hiding this comment.
for is more about responsibilities, and for me there is no need to log as a convenience. The log has to be printed in the StashStep if verbose. And StashManager to provide a list a changes.
There was a problem hiding this comment.
At least pass the verbose flag into the StashManager methods and let the implementation handle the printing.
| <dependency> | ||
| <groupId>org.jenkins-ci.plugins.workflow</groupId> | ||
| <artifactId>workflow-step-api</artifactId> | ||
| <version>2.23</version> |
There was a problem hiding this comment.
Need to retain TODO comments indicating that this should be removed once in the BOM
| <dependency> | ||
| <groupId>org.jenkins-ci.plugins.workflow</groupId> | ||
| <artifactId>workflow-api</artifactId> | ||
| <version>2.41-SNAPSHOT</version> |
There was a problem hiding this comment.
Do not use snapshots. https://github.com/jenkinsci/incrementals-tools
| <artifactId>scm-api</artifactId> | ||
| <version>2.6.4</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Very likely not what you wanted to do. If you ran into a RequireUpperBoundsDeps error because you went outside the BOM, add to dependencyManagement (not dependencies) and mark with a TODO comment to remove as soon as possible.
| r.assertLogContains("stashed file /p/p/p/other", b); | ||
| r.assertLogContains("unstashed file /p/elsewhere/fname", b); | ||
| r.assertLogContains("unstashed file /p/elsewhere/other", b); | ||
| assertEquals("{}", StashManager.stashesOf(b).toString()); // TODO flake expected:<{[]}> but was:<{[from-top={elsewhere/fname=whatever}, whatever={fname=whatever, other=more}]}> |
| String name = getContext().get(Run.class).getParent().getName(); | ||
| logger = getContext().get(TaskListener.class).getLogger(); | ||
| String gibberish = file.getCanonicalPath().split("/" + name + "/")[0]; | ||
| logger.println("stashed file " + StringUtils.substring(file.getCanonicalPath(), gibberish.length())); |
There was a problem hiding this comment.
Why the absolute paths? Everything should be using relative paths here.
|
|
||
| @Override | ||
| protected Void run() throws Exception { | ||
| List<File> stash = StashManager.stash(getContext().get(Run.class), step.name, getContext().get(FilePath.class), getContext().get(Launcher.class), getContext().get(EnvVars.class), getContext().get(TaskListener.class), step.includes, step.excludes, |
There was a problem hiding this comment.
File? Does not smell right. That could only refer to paths on the controller, and stashes are archived into a tarball. If these “files” are on an agent, then you cannot use File to refer to them. You could use FilePath but (as above) you probably just meant to use String (relative paths).
There was a problem hiding this comment.
my goal is to pass something neutral, like a file (or FilePAth), or even a String. I did passed a File in order to provide more info in higher levels, not meaning to pass the actual file. Indeed, something like a DTO, which could be created.
There was a problem hiding this comment.
Just String should suffice. stash and unstash deal with paths relative to the contextual working dir, so the absolute file path is not really interesting.
|
See #54. |
|
Obsolete I guess? |
3 participants
No description provided.