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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JENKINS-49322] Add an optional prefix parameter to artifact archiver #3284

Closed
wants to merge 1 commit into from
Closed

[JENKINS-49322] Add an optional prefix parameter to artifact archiver #3284

wants to merge 1 commit into from

Conversation

segalaj
Copy link
Contributor

@segalaj segalaj commented Feb 9, 2018

Add targetDirectory parameter for the artifact archiver
No test added, I can add some if needed.

See JENKINS-49322.

Proposed changelog entries

  • Add an optional parent directory parameter to the artifact archiver

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

no idea, sorry 馃槈

@segalaj
[JENKINS-49322] Add an optional prefix parameter to artifact archiver
611e7a5 
 - add targetDirectory parameter for the archive
@daniel-beck
Copy link
Member

daniel-beck commented Feb 9, 2018 •
edited

Alternative to #1878, as JENKINS-49322 duplicates JENKINS-12379.

Not true: Actually this allows configuring where on the Jenkins master stuff gets archived.

@daniel-beck daniel-beck added needs-more-reviews Complex change, which would benefit from more eyes needs-testcase Test automation is required for this pull request labels Feb 9, 2018
daniel-beck
daniel-beck requested changes Feb 9, 2018
View reviewed changes
core/src/main/java/jenkins/model/ArtifactManager.java
* @throws IOException if transfer or copying failed in any way
* @throws InterruptedException if transfer was interrupted
* @see ArtifactArchiver#perform(Run, FilePath, Launcher, TaskListener)
*/
public abstract void archive(FilePath workspace, Launcher launcher, BuildListener listener, Map<String,String> artifacts) throws IOException, InterruptedException;
public abstract void archive(FilePath workspace, Launcher launcher, BuildListener listener, Map<String,String> artifacts, String targetDirectory) throws IOException, InterruptedException;
Copy link
Member

Choose a reason for hiding this comment

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

Change breaking API compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@daniel-beck I agree. Should I add another method signature or find antoher way that avoid modifying the API?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Add another method with more arguments, and invoke that one from this one with whatever is the "default" argument.

@daniel-beck daniel-beck added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Feb 9, 2018
@batmat
Copy link
Member

batmat commented Mar 19, 2019

This is conflicted, has a change request from Daniel, and last comment was 1+ year ago.

@segalaj do you plan to get back to this? No worries if not, this happens :). Just trying to push open PRs to completion.

I will propose this for closing and will close it in the next days if the status doesn't change.

Thanks!

@batmat batmat added unresolved-merge-conflict There is a merge conflict with the target branch. stalled The PR is reasonable and might be merged, but it is no longer active. It can be taken over by other labels Mar 19, 2019
@segalaj
Copy link
Contributor Author

segalaj commented Mar 19, 2019

@batmat thanks for your comment, not sure I'll come back on this. I was expecting an answer to my last comment to continue.

@daniel-beck
Copy link
Member

I was expecting an answer to my last comment to continue.

Sorry about that, I missed your response.

@batmat batmat requested review from daniel-beck and removed request for daniel-beck April 8, 2019 20:24
@batmat batmat removed the stalled The PR is reasonable and might be merged, but it is no longer active. It can be taken over by other label Apr 8, 2019
@batmat
Copy link
Member

batmat commented Apr 11, 2019

@segalaj Daniel was able to answer in #3284 (comment). Sorry again for the miss.

Do you think you want to keep this open, so you can finalize it, or you'd prefer to close it even temporarily (and we can reopen anytime you want to get back to it)?

Thank you very much and sorry again.

@batmat
Copy link
Member

batmat commented Apr 22, 2019

Given this PR is conflicted, and I didn't get answer from my last comment 11 days, I am going to close it.

Please feel free to comment so we reopen this PR anytime once/if you are available again. Or filing a new PR depending on your preference.

Thanks a lot.

@batmat batmat closed this Apr 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daniel-beck daniel-beck daniel-beck requested changes

Assignees
No one assigned
Labels
needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging needs-more-reviews Complex change, which would benefit from more eyes needs-testcase Test automation is required for this pull request unresolved-merge-conflict There is a merge conflict with the target branch.
Projects
None yet
Milestone
No milestone
3 participants
@segalaj @daniel-beck @batmat