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.

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-14155]Use the lastest changed tag as default #157

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MichaWiedenmann
Copy link

In a recent commit (5012326 - [JENKINS-14155] Automatically select the first option of tags listing…"), support was added to select the latest changed tag as default value. For this to work, the repository URL (of the subversion tag parameters) must point to the tags directory (e.g. foo/tags). If you want to also include trunk and branches (i.e. setting the repository URL to foo/), you cannot benefit from the "use the latest tag" feature.

This branch extends the recent functionality, such that one can use a repository URL pointing to the top level directory (foo) while still using the latest changed tag from the tags sub directory as default.

To this end a new checkbox "Use latest tag" is added below the "Default value" text box.

@jenkinsadmin
Copy link
Member

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

jglick
jglick previously requested changes Jun 16, 2017
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

merge conflict

@kuisathaverat
Copy link
Contributor

Closed and Open to relaunch CI

@kuisathaverat kuisathaverat reopened this May 8, 2018
@kuisathaverat kuisathaverat changed the title Use the lastest changed tag as default [JENKINS-14155]Use the lastest changed tag as default May 8, 2018
Copy link
Contributor

@Wadeck Wadeck left a comment

Choose a reason for hiding this comment

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

Just a concern about the binary compatibility

}

@DataBoundConstructor
public ListSubversionTagsParameterDefinition(String name, String tagsDir, String credentialsId, String tagsFilter, String defaultValue, String maxTags, boolean reverseByDate, boolean reverseByName) {
public ListSubversionTagsParameterDefinition(String name, String tagsDir, String credentialsId, String tagsFilter, String defaultValue, boolean useLatestTag, String maxTags, boolean reverseByDate, boolean reverseByName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I dunno exactly the guideline for this plugin, but from my PoV it's better to use @DataBoundSetter when you add field instead of putting that in the constructor.

private final String maxTags;
private static final String SVN_BRANCHES = "branches";
private static final String SVN_TAGS = "tags";
private static final String SVN_TRUNK = "trunk";

@Deprecated
public ListSubversionTagsParameterDefinition(String name, String tagsDir, String tagsFilter, String defaultValue, String maxTags, boolean reverseByDate, boolean reverseByName, String uuid) {
this(name, tagsDir, null, tagsFilter, defaultValue, maxTags, reverseByDate, reverseByName);
public ListSubversionTagsParameterDefinition(String name, String tagsDir, String tagsFilter, String defaultValue, boolean useLatestTag, String maxTags, boolean reverseByDate, boolean reverseByName, String uuid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Those additions will result in breaking the binary compatibility. Is it required to pass by the constructor ? I think a simple use of a setter / putting a default value would be sufficient.

@@ -173,6 +181,10 @@ public DescriptorImpl getDescriptor() {
return (DescriptorImpl) super.getDescriptor();
}

@Nonnull public List<String> getTags(@Nullable Job context) {
return getTags(context, "", isReverseByDate());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 to think about compatibility

Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

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

Feel free to request my review again if Wadeck's comments are addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants