[JENKINS-43507] Allow SCMSource and SCMNavigator subtypes to share common traits #103

stephenc · 2017-05-04T13:36:21Z

Downstream of jenkinsci/scm-api-plugin#36

See JENKINS-43507

@reviewbybees

Todo

Ensure that the trait implementations work correctly when BitBucket branch source picks up traits

stephenc · 2017-05-04T13:38:11Z

Screenshot the maximum bling new configuration (showing the shared traits for matching branch names)

ghost · 2017-05-04T13:38:57Z

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.

jglick

A few little mistakes but basically sounds good.

jglick · 2017-05-05T17:18:44Z

pom.xml

@@ -7,7 +7,7 @@
        <relativePath />
    </parent>
    <artifactId>mercurial</artifactId>
-    <version>1.61-SNAPSHOT</version>
+  <version>2.0-SNAPSHOT</version>


Why the outdentation?

jglick · 2017-05-05T17:21:42Z

src/main/java/hudson/plugins/mercurial/MercurialSCMBuilder.java

+        result.setClean(clean());
+        result.setCredentialsId(credentialsId());
+        result.setInstallation(installation());
+        result.setDisableChangeLog(false);


Why? This is the default.

jglick · 2017-05-05T17:22:28Z

src/main/java/hudson/plugins/mercurial/MercurialSCMSource.java

 import hudson.EnvVars;
 import hudson.Extension;
 import hudson.FilePath;
 import hudson.Launcher;
+import hudson.RestrictedSince;


Hmm, maybe this should be moved into access-restrictions-annotation.

jglick · 2017-05-05T17:23:45Z

src/main/java/hudson/plugins/mercurial/MercurialSCMSource.java


    @DataBoundConstructor
-    public MercurialSCMSource(String id, String installation, String source, String credentialsId, String branchPattern, String modules, String subdir, HgBrowser browser, boolean clean) {
+    public MercurialSCMSource(String id, String source, String credentialsId) {


Even credentialsId could be in a @DataBoundSetter. null is a legal value.

jglick · 2017-05-05T17:29:11Z

src/main/java/hudson/plugins/mercurial/MercurialSCMSource.java

+                                @Override
+                                public @Nonnull
+                                SCMSourceCriteria.Probe create(@Nonnull SCMHead branch, @Nullable final
+                                MercurialRevision revision) {


Do you have your IDE set to some ridiclously small fill column or something? This method header could all be on one line…

jglick · 2017-05-05T17:32:55Z

src/main/java/hudson/plugins/mercurial/traits/CleanMercurialSCMSourceTrait.java

+    @DataBoundConstructor public CleanMercurialSCMSourceTrait() { }
+
+    @Override protected <B extends SCMBuilder<B, S>, S extends SCM> void decorateBuilder(B builder) {
+        ((MercurialSCMBuilder<?>) builder).withClean(true);


Why the exotic typing of the method signature if we are going to wind up casting anyway?

jglick · 2017-05-05T17:41:50Z

src/main/java/hudson/plugins/mercurial/traits/MercurialBrowserSCMSourceTrait.java

@@ -0,0 +1,121 @@
+/*
+ * The MIT License


BTW license is present on some, but not all, newly added files.

should be fixed now

jglick · 2017-05-05T17:41:58Z

src/main/java/hudson/plugins/mercurial/traits/MercurialBrowserSCMSourceTrait.java

+/**
+ * Exposes {@link HgBrowser} configuration of a {@link MercurialSCMSource} as a {@link SCMSourceTrait}.
+ *
+ * @since 3.4.0


jglick · 2017-05-05T17:44:08Z

src/main/java/hudson/plugins/mercurial/traits/MercurialInstallationSCMSourceTrait.java

+        @Override public boolean isApplicableToBuilder(@Nonnull Class<? extends SCMBuilder> builderClass) {
+            if (super.isApplicableToBuilder(builderClass) && MercurialSCMBuilder.class.isAssignableFrom(builderClass)) {
+                for (MercurialInstallation i : MercurialInstallation.allInstallations()) {
+                    if (i.isUseCaches()) {


Well…if you have no installations set to useCaches then you cannot use MercurialSCMSource at all. So whenever this method is called, it should be returning true.

jglick · 2017-05-05T17:46:00Z

src/main/resources/hudson/plugins/mercurial/traits/Messages.properties

@@ -0,0 +1 @@
+MercurialBrowserSCMSourceTrait.displayName=Configure Repository Browser


Not actually used.

jglick · 2017-06-15T18:05:33Z

test failures

jglick

Uncompilable.

jglick

Some nits, and diff could have been made shorter, but overall looks good.

Please do not rebase commits again if you can help it.

jglick · 2017-06-19T21:26:09Z

pom.xml

@@ -69,9 +70,23 @@
            <url>https://repo.jenkins-ci.org/public/</url>
        </pluginRepository>
    </pluginRepositories>
+    <dependencyManagement>


Why? We are already reusing the property. And in my experience, dependencyManagement does not in fact work to align versions of artifacts with different classifiers.

jglick · 2017-06-19T21:26:41Z

pom.xml

+                <groupId>org.jenkins-ci.tools</groupId>
+                <artifactId>maven-hpi-plugin</artifactId>
+                <configuration>
+                    <compatibleSinceVersion>2.0</compatibleSinceVersion>


What specifically is incompatible? I will need to have a list for release notes. It seems like you are updating everything relevant in readResolve so what am I missing?

This is the flag that the data format has changed and you cannot downgrade after upgrading without a backup

Hmm, that is generally true of any plugin release. Certainly we do not routinely add this attribute. It adds a warning to the update center which will be alarming.

Really... that is the stated purpose of the attribute and I use it every time there is a field migration... adding new fields is not a warning

https://github.com/jenkinsci/maven-hpi-plugin/blob/b3052ae8d3204b0f6630c94c4d7ed930443f6007/src/main/java/org/jenkinsci/maven/plugins/hpi/AbstractHpiMojo.java#L114-L117

the oldest version of this plugin which the current version is configuration-compatible with

If we add new additional fields (the majority of the time changes in plugins are this category), on downgrade the new fields will be silently ignored

If we migrate fields - which is what we are doing here - then the old field will no longer be available after the job has been saved, which is exactly a break in configuration compatibility, which is why this is the marker in the sand

jglick · 2017-06-19T21:27:29Z

src/main/java/hudson/plugins/mercurial/MercurialSCMBuilder.java

+    String source;
+
+    public MercurialSCMBuilder(@Nonnull SCMHead head, @CheckForNull SCMRevision revision, String source,
+                               String credentialsId) {


Nullability—source nonnull, credentialsId OK?

jglick · 2017-06-19T21:27:58Z

src/main/java/hudson/plugins/mercurial/MercurialSCMBuilder.java

+        return (B) this;
+    }
+
+    @SuppressWarnings("unchecked") public @Nonnull B withCredentialsId(String credentialsId) {


Wait then why include it in the constructor?

because you are supposed to provide the default credentials in the constructor... then traits can decorate to switch to alternative credentials

jglick · 2017-06-19T21:40:36Z

src/main/java/hudson/plugins/mercurial/MercurialSCMSource.java

-        return modules;
+    @Deprecated @Restricted(DoNotUse.class) @RestrictedSince("2.0") public String getInstallation() {
+        for (SCMSourceTrait t: traits) {
+            if (t instanceof MercurialInstallationSCMSourceTrait) {


Would be useful to have a

<T extends SCMSourceTrait> public List<? extends T> getTraits(Class<T> type);

jglick · 2017-06-19T21:41:18Z

src/main/java/hudson/plugins/mercurial/MercurialSCMSource.java

    }

-    public String getModules() {
-        return modules;
+    @Deprecated @Restricted(DoNotUse.class) @RestrictedSince("2.0") public String getInstallation() {


@Restricted is unnecessary. @Deprecated suffices.

Restrictions let up use plugin version stats to guide removing old code. Depreciation does not force dependent plugins to update.

I am making the path to removing the compatibility methods easier to reach

The deprecated methods need to remain for a couple years either way, and do little harm.

jglick · 2017-06-19T21:43:24Z

src/main/java/hudson/plugins/mercurial/MercurialSCMSource.java

+            Launcher launcher = node.createLauncher(listener);
+            StandardUsernameCredentials credentials = getCredentials(request.credentialsId());
+            final FilePath cache = Cache.fromURL(source, credentials, inst.getMasterCacheRoot())
+                    .repositoryCache(inst, node, launcher, listener, true);


Did you somehow contrive to make this diff as long as possible? Would much rather see this diff hunk disappear, and the try block simply not be indented.

jglick · 2017-06-19T21:44:31Z

src/main/java/hudson/plugins/mercurial/MercurialSCMSource.java

-    @NonNull
-    @Override
-    protected List<Action> retrieveActions(@NonNull SCMHead head,
+    @Override protected @Nonnull List<Action> retrieveActions(@NonNull SCMHead head,


Gratuitous diff hunk.

aligning with annotation conventions... can remove but then I wouldn't be practicing why I preached ;-)

jglick · 2017-06-19T21:44:58Z

src/main/java/hudson/plugins/mercurial/MercurialSCMSource.java

-        private boolean hasAccessToCredentialsMetadata(SCMSourceOwner owner){
-            if (owner == null){
+        private boolean hasAccessToCredentialsMetadata(SCMSourceOwner owner) {
+            if (owner == null) {


Please stop wantonly reformatting source files!

jglick · 2017-06-20T15:46:28Z

src/main/java/hudson/plugins/mercurial/MercurialSCMSource.java

+        return getTrait(CleanMercurialSCMSourceTrait.class) != null;
+    }
+
+    private @CheckForNull <T extends SCMSourceTrait> T getTrait(@Nonnull Class<T> traitClass) {


I am to take it that it is illegal to have multiple instances of the same trait class? I guess you are using the corresponding control mode in scm-api?

jglick · 2017-06-20T15:47:02Z

src/main/resources/hudson/plugins/mercurial/MercurialSCMSource/config-detail_en_US.properties

@@ -0,0 +1 @@
+Behaviours=Behaviors


you have got to be kidding me

jglick · 2017-08-24T14:08:19Z

src/main/java/hudson/plugins/mercurial/MercurialSCMSource.java

+                return;
+            }
+            Launcher launcher = node.createLauncher(listener);
+            StandardUsernameCredentials credentials = getCredentials(request.credentialsId());


I think this regressed credentials handling: #108. Means that authenticated repository access is not being tested.

stephenc requested review from jglick and rsandell May 4, 2017 13:36

jglick added the work-in-progress label May 5, 2017

jglick approved these changes May 5, 2017

View reviewed changes

stephenc mentioned this pull request Jun 12, 2017

[JENKINS-43507] Migrate Bitbucket Plugin to Traits jenkinsci/bitbucket-branch-source-plugin#53

Merged

stephenc added 6 commits June 12, 2017 22:24

[JENKINS-43507] Adapt MercurialSCMSource to use traits

fe7b13e

[JENKINS-43507] Remove dead help files and restore live one

a87b60a

[JENKINS-43507] Flag that the data model changes and bump to 2.0

18514a4

[JENKINS-43507] Fix check of repository source

e09089d

[JENKINS-43507] Fix optionality of configuration

fc22556

[JENKINS-43507] Update to latest -SNAPSHOT of scm-api

d41c751

stephenc force-pushed the jenkins-43507 branch from 718d644 to d41c751 Compare June 12, 2017 21:24

stephenc removed the work-in-progress label Jun 14, 2017

[JENKINS-43507] Pick up changes for JENKINS-44891

b380c2f

stephenc force-pushed the jenkins-43507 branch from fc2e5b7 to b380c2f Compare June 15, 2017 14:37

[JENKINS-43507] Pick up structs 1.8

c519a25

rsandell mentioned this pull request Jun 19, 2017

[JENKINS-33701] Set scm browser on mercurial and git repos jenkinsci/bitbucket-branch-source-plugin#36

Closed

Merge remote-tracking branch 'origin/master' into jenkins-43507

0451b4c

jglick requested changes Jun 19, 2017

View reviewed changes

Fix merge errors

710f0f7

jglick approved these changes Jun 19, 2017

View reviewed changes

stephenc added 6 commits June 20, 2017 10:17

[JENKINS-43507] Code review comments

900cbd0

[JENKINS-43507] I18n

f083196

[JENKINS-43507] Copyright headers

58a6595

[JENKINS-43507] Plugin maintainer doesn't want

075a1ba

[JENKINS-43507] Plugin requested formatting changes

e8bbfc9

[JENKINS-43507] Whitespace

01fe831

stephenc added 4 commits June 20, 2017 12:31

[JENKINS-43507] Pick up -alpha-1

26f4d28

[maven-release-plugin] prepare release mercurial-2.0-alpha-1

cd1c60a

[maven-release-plugin] prepare for next development iteration

c1aa0d8

[JENKINS-43507] US people want to spell things incorrectly

d632ecb

jglick approved these changes Jun 20, 2017

View reviewed changes

stephenc added 4 commits June 24, 2017 08:44

[JENKINS-43507] Fix trait drop-down list population

b1bc5a4

[maven-release-plugin] prepare release mercurial-2.0-alpha-4

d25adbd

[maven-release-plugin] prepare for next development iteration

117a6e5

[JENKINS-43507] Bump structs to 1.9

120115b

jglick approved these changes Jun 26, 2017

View reviewed changes

stephenc merged commit bfc7cbd into jenkinsci:master Jun 27, 2017

stephenc deleted the jenkins-43507 branch June 27, 2017 08:41

jglick reviewed Aug 24, 2017

View reviewed changes

jglick mentioned this pull request Aug 24, 2017

[JENKINS-45806] withCredentialsId needed #111

Merged

		@@ -0,0 +1 @@
		MercurialBrowserSCMSourceTrait.displayName=Configure Repository Browser

		@@ -0,0 +1 @@
		Behaviours=Behaviors

[JENKINS-43507] Allow SCMSource and SCMNavigator subtypes to share common traits #103

[JENKINS-43507] Allow SCMSource and SCMNavigator subtypes to share common traits #103

Conversation

stephenc commented May 4, 2017 • edited Loading

stephenc commented May 4, 2017

ghost commented May 4, 2017

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Jun 15, 2017

jglick left a comment

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenc commented May 4, 2017 •

edited

Loading