Add `includeBuildNumberInTargetPath` configuration parameter #169

allancth · 2023-01-27T07:43:15Z

Add `includeBuildNumberInTargetPath` configuration parameter

The new parameter includeBuildNumberInTargetPath accepts a boolean. When true, the build number of the source project will be included the target path. This is particularly useful when the selector is specific and the value is a permalink, e.g. lastSuccessfulBuild.

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

src/main/resources/hudson/plugins/copyartifact/CopyArtifact/help-appendSrcNumberToTarget.html

MarkEWaite · 2023-01-27T16:33:30Z

I'd much prefer that there was a test that exercises the new code and confirms it works as expected. Would you be willing to add an automated test to confirm it?

allancth · 2023-01-28T01:13:29Z

Yeap I can do that. Is there a test case I can refer to as reference? Regards, Allan

…

On Sat, Jan 28, 2023, 00:33 Mark Waite ***@***.***> wrote: I'd much prefer that there was a test that exercises the new code and confirms it works as expected. Would you be willing to add an automated test to confirm it? — Reply to this email directly, view it on GitHub <#169 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2JCEZE4GJQPZ2DIBVBNSDWUP2GLANCNFSM6AAAAAAUIMNKQQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

MarkEWaite · 2023-01-28T04:58:07Z

I can do that. Is there a test case I can refer to as reference? Regards, Allan

copyartifact-plugin/src/test/java/hudson/plugins/copyartifact/CopyArtifactTest.java

Line 2253 in aa298c7

@Test

seems like a good match for something similar to what this new feature is doing.

allancth · 2023-01-28T07:36:07Z

The parameter name is a bit misleading but I can't think of one better. Let me know if there are things that could improve this pull request, I'll gladly change. Thanks in advance.

MarkEWaite · 2023-01-28T17:15:03Z

The parameter name is a bit misleading but I can't think of one better. Let me know if there are things that could improve this pull request, I'll gladly change. Thanks in advance.

I agree that the parameter name is a bit misleading. While I was interactively testing it, I was surprised when the build number was used as the directory name. That seemed like "prepending" rather than "appending".

Would it be clearer if it was named includeBuildNumberInTargetPath or something similar?

… test

allancth · 2023-01-29T02:41:05Z

Thanks Mark for the suggestion. I've updated the name of the parameter and the test case.

src/main/webapp/help-flatten-optional.html

MarkEWaite · 2023-01-29T03:32:00Z

Thanks very much @allancth. The pull request is looking good.

In future pull requests, you can simplify things for the maintainer if you create the pull request from a feature branch rather than using the master branch. When starting development on a fresh feature, sync your repository with the upstream repository and then create a new branch with git checkout -b new-feature-name master

Also in future pull requests, you can simplify things for the maintainer if you don't remove the pull request checklist that GitHub inserts into the pull request. That checklist reminds the maintainer of things to check and can remind you of things to do as well.

allancth · 2023-01-29T03:42:59Z

Noted Mark. I'll make sure to do that in the future. Appreciate the advice and guidance. Cheers. Regards, Allan

…

On Sun, Jan 29, 2023, 11:32 Mark Waite ***@***.***> wrote: Thanks very much @allancth <https://github.com/allancth>. The pull request is looking good. In future pull requests, you can simplify things for the maintainer if you create the pull request from a feature branch rather than using the master branch. When starting development on a fresh feature, sync your repository with the upstream repository and then create a new branch with git checkout -b new-feature-name master Also in future pull requests, you can simplify things for the maintainer if you don't remove the pull request checklist that GitHub inserts into the pull request. That checklist reminds the maintainer of things to check and can remind you of things to do as well. — Reply to this email directly, view it on GitHub <#169 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2JCEZY2UYYCKN6QPT3ROTWUXQDXANCNFSM6AAAAAAUIMNKQQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

MarkEWaite

Thanks again for the contribution. Ready to squash and merge. My interactive testing showed this addition was well behaved in all the cases that I tested.

MarkEWaite · 2023-01-29T13:39:04Z

Released as 684.vde867d759462

MarkEWaite · 2023-01-29T13:41:04Z

@allancth are you interested in adopting the copyartifact plugin? The plugin has been modernized enough that the work to be a maintainer is probably no more than an hour or two a month to review pull requests for dependency updates.

allancth · 2023-01-30T01:10:02Z

Thanks @MarkEWaite. I am interested in adopting and be happy to be a maintainer for the plugin. I have not done anything like this before hence it would be helpful if there are guides I can use.

MarkEWaite · 2023-01-30T01:20:08Z

There are guides that can help. The steps are described in "Adopt a plugin" where you send email to the Jenkins developers list saying that you'd like to adopt the plugin and submit a pull request to the permissions repository that will add yourself to the list of maintainers. You need to login to https://issues.jenkins.io with your Jenkins account and you need to login to https://repo.jenkins-ci.org with your Jenkins account.

I'm happy to help with the steps if you encounter issues.

The "improve a plugin" tutorial includes videos and steps that can help (though most are already done for this plugin). If you'd like more ideas for improvements, there is the "Contributing to Open Source" document

daniel-beck · 2023-01-30T12:44:15Z

src/main/resources/hudson/plugins/copyartifact/CopyArtifact/config_de.properties

This will translate the strings to an empty string. Almost certainly not what you want.

Thanks @daniel-beck . I saw that in the code review and wondered about it. When I searched through Jenkins core and saw a number of message properties that were assigned an empty value, I thought that it must be a pattern that caused the property to be assigned the value from the default language.

An example:

https://github.com/jenkinsci/jenkins/blob/a530a99926940cb37fd2f7b66e5c2f458254a710/core/src/main/resources/hudson/logging/LogRecorder/configure_fr.properties#L24

Another example:

https://github.com/jenkinsci/jenkins/blob/a530a99926940cb37fd2f7b66e5c2f458254a710/core/src/main/resources/hudson/model/RunParameterDefinition/config_fr.properties#L24

Is the preferred pattern to leave the untranslated key out of the localized property file or is there a better way to handle it?

In this case, the best way to handle it may be to transition this plugin to use CrowdIn for its localization. That will allow translators to submit messages through the CrowdIn user interface rather than directly editing property files.

@allancth would you like to submit the pull request to remove the untranslated properties or would you prefer that I do it so that you can approve the change?

@allancth I submitted:

Remove empty translation properties #171

If you approve and merge, it should result in a new release with a changelog that includes the bug fix.

daniel-beck · 2023-01-30T12:44:25Z

src/main/resources/hudson/plugins/copyartifact/CopyArtifact/config_ja.properties

#169 added empty translation strings. They will cause the two items to be shown as empty strings in the local language versions (German and Japanese). Better to remove the empty translation properties so that the English will be displayed.

allancth · 2023-01-30T15:41:39Z

Sorry Mark for the late reply and not able to submit a PR earlier. I have approved the PR you've submitted. Thank you. Regards, Allan

…

On Mon, Jan 30, 2023, 21:15 Mark Waite ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/resources/hudson/plugins/copyartifact/CopyArtifact/config_de.properties <#169 (comment)> : > +Fingerprint\ Artifacts= +Include\ Build\ Number= Thanks @daniel-beck <https://github.com/daniel-beck> . I saw that in the code review and wondered about it. When I searched through Jenkins core and saw a number of message properties that were assigned an empty value, I thought that it must be a pattern that caused the property to be assigned the value from the default language. An example: https://github.com/jenkinsci/jenkins/blob/a530a99926940cb37fd2f7b66e5c2f458254a710/core/src/main/resources/hudson/logging/LogRecorder/configure_fr.properties#L24 Is the preferred pattern to leave the untranslated key out of the localized property file or is there a better way to handle it? In this case, the best way to handle it may be to transition this plugin to use CrowdIn for its localization. That will allow translators to submit messages through the CrowdIn user interface rather than directly editing property files. @allancth <https://github.com/allancth> would you like to submit the pull request to remove the untranslated properties or would you prefer that I do it so that you can approve the change? — Reply to this email directly, view it on GitHub <#169 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2JCE3UP7GA4PQFDUBRII3WU65INANCNFSM6AAAAAAUIMNKQQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Add parameter appendSrcNumberToTarget

01d8d55

allancth requested a review from a team as a code owner January 27, 2023 07:43

MarkEWaite reviewed Jan 27, 2023

View reviewed changes

src/main/resources/hudson/plugins/copyartifact/CopyArtifact/help-appendSrcNumberToTarget.html Outdated Show resolved Hide resolved

allancth added 3 commits January 28, 2023 14:09

Fix field name for appendSrcNumberToTarget

02e4fa6

Incorporate suggestion

8420660

Unit test for parameter appendSrcNumberToTarget

bf97bef

MarkEWaite added 2 commits January 28, 2023 09:38

Remove trailing space from a line

05b75ac

Make help visible in freestyle project

c16435f

MarkEWaite added the enhancement New feature or improvement label Jan 28, 2023

allancth added 4 commits January 29, 2023 10:33

Change parameter name to includeBuildNumberInTargetPath + update unit…

2d6193e

… test

Change parameter name to includeBuildNumberInTargetPath + update unit…

785b692

… test

Merge branch 'master' of github.com:allancth/copyartifact-plugin

75f9582

Change wordings: *Append* to *Include*

adb22b1

MarkEWaite changed the title ~~New configuration parameter appendSrcNumberToTarget~~ New configuration parameter includeBuildNumberInTargetPath Jan 29, 2023

Update parameter name

7c53835

MarkEWaite changed the title ~~New configuration parameter includeBuildNumberInTargetPath~~ New configuration parameter includeBuildNumberInTargetPath Jan 29, 2023

MarkEWaite changed the title ~~New configuration parameter includeBuildNumberInTargetPath~~ Add includeBuildNumberInTargetPath configuration parameter Jan 29, 2023

MarkEWaite approved these changes Jan 29, 2023

View reviewed changes

src/main/webapp/help-flatten-optional.html Outdated Show resolved Hide resolved

MarkEWaite added 3 commits January 28, 2023 20:23

One more phrasing change

1f0a02d

Match help file name to field name

1063876

Merge branch 'master' into allancth/master

c322d9c

MarkEWaite approved these changes Jan 29, 2023

View reviewed changes

MarkEWaite merged commit de867d7 into jenkinsci:master Jan 29, 2023

daniel-beck reviewed Jan 30, 2023

View reviewed changes

MarkEWaite mentioned this pull request Jan 30, 2023

Add new maintainer for copyartifact-plugin jenkins-infra/repository-permissions-updater#3108

Merged

9 tasks

MarkEWaite mentioned this pull request Jan 30, 2023

Remove empty translation properties #171

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `includeBuildNumberInTargetPath` configuration parameter #169

Add `includeBuildNumberInTargetPath` configuration parameter #169

allancth commented Jan 27, 2023 •

edited by MarkEWaite

Loading

MarkEWaite commented Jan 27, 2023

allancth commented Jan 28, 2023 via email

MarkEWaite commented Jan 28, 2023

allancth commented Jan 28, 2023

MarkEWaite commented Jan 28, 2023

allancth commented Jan 29, 2023

MarkEWaite commented Jan 29, 2023

allancth commented Jan 29, 2023 via email

MarkEWaite left a comment

MarkEWaite commented Jan 29, 2023

MarkEWaite commented Jan 29, 2023

allancth commented Jan 30, 2023

MarkEWaite commented Jan 30, 2023

daniel-beck Jan 30, 2023

MarkEWaite Jan 30, 2023 •

edited

Loading

MarkEWaite Jan 30, 2023

daniel-beck Jan 30, 2023

allancth commented Jan 30, 2023 via email

Add includeBuildNumberInTargetPath configuration parameter #169

Add includeBuildNumberInTargetPath configuration parameter #169

Conversation

allancth commented Jan 27, 2023 • edited by MarkEWaite Loading