Implement setters instead of constructor for optional parameters #246

westarne · 2019-10-02T15:17:24Z

Concerning #245

I was not able to reproduce the original issue, but comparing with other plugins this might do the trick. For now this was not tested by me on an actual Jenkins and I could only test if it works for me as it did before. It would be great if someone, for whom the v1.20 is failing, could test this build. (@adrian-at-dmi, @tschlue, @r4d1um)

If you are not able to build this plugin (and you trust me), here is a locally built .hpi:
stashNotifier.zip

Fixes #245

westarne · 2019-10-04T08:18:08Z

I have now tested this build on my Jenkins and it still works for me. So it does not seem to break anything at least. But I cannot verify whether this actually makes the parameters optional as intended, but it should...

scaytrase · 2019-10-04T08:46:57Z

@westarne it does not break already configured instances? I'll try to test it on my jenkins instance on weekend

westarne · 2019-10-04T08:49:28Z

I've tested this via manually replacing the old *.jpi in the plugins folder, so that worked without changing anything configured. I'm not sure if this would work via the usual install process as well, but I think it should.

scaytrase · 2019-10-04T08:50:08Z

sounds good

proski · 2019-12-03T07:22:47Z

I believe this change will make it possible to use notifyBitbucket without arguments in declarative pipelines. That would be very good! I can do it by surrounding that command with script { ... }, but it shouldn't be necessary. There are no required arguments if the URL and credentials are set up in the global settings.

However, I'm concerned that this PR appears to be changing more code than necessary. In particular, it should not be adding extra settings. It's better to keep the configuration to the minimum. Even some of the existing overrides don't make sense to me. For instance, "Override project key". Why would anyone do it? And this PR is adding "Build Status". Why would anyone use anything but the actual build result?

In any case, adding new user interface should be a separate PR, with a separate justification.

westarne · 2019-12-03T07:40:07Z

Both project key and build status are useful and already implemented and merged from different previous issues.
They are not added in this PR.

~~Override the project key is useful to push separate statuses for different build steps in the pipeline as well as the build status.~~ (this is actually the buildName not projectKey i meant)

You can this way use the custom buildStatus for example to say that the build step was successful and the test step is in progress. By setting the buildStatus to succesful once you could otherwise never push a "in progress" anymore after that. But as i said, both were implemented before and not in this PR.

scaytrase · 2019-12-03T07:43:20Z

Yes, that's true, override project key and build status were implemented before

src/main/resources/org/jenkinsci/plugins/stashNotifier/StashNotifier/config.jelly

darxriggs · 2020-01-06T18:11:59Z

I just saw that there is a merge conflict to be resolved.

So let me ask what is the current state of this pull request and the next steps?

scaytrase · 2020-01-06T18:27:09Z

So let me ask what is the current state of this pull request and the next steps?

On #245 people report problem is gone when they use script wrapper, so I'm not sure we are fixing a problem here. But as an improvement this might be good

westarne · 2020-01-06T18:28:05Z

I've resolved the conflicts.

In my opinion this change could fix the issue #245. But I was not able to really test this because I could not reproduce the initial issue. It'd great if someone could test this manually (e.g. with the provided .hpi) who is able to reproduce the initial issue.

In general this should not create additional issues either way even if it does not fix #245 but testing it in integration first is still important I think.

darxriggs · 2020-01-06T19:59:24Z

@westarne Do you know mvn hpi:run? It is very handy to interactively try out the latest changes during development.

I suggest to run it, install the "Pipeline: Declarative" plugin and try a Jenkinsfile like this:

pipeline {
   agent any

   stages {
      stage('Testing') {
         steps {
            echo 'Some Message'
            notifyBitbucket(
                stashServerBaseUrl: 'https://localhost',
                projectKey: 'KEE',
                commitSha1: 'shaaaaaaaa',
                credentialsId: 'example-id',
                considerUnstableAsSuccess: false,
                disableInprogressNotification: false,
                ignoreUnverifiedSSLPeer: false,
                includeBuildNumberInKey: false,
                prependParentProjectKey: false,
            )
         }
      }
   }
}

You should be able to reproduce the issue with this pipeline on the current master or latest tag 1.20 state and also be able to verify in your branch that it is fixed.

westarne · 2020-01-06T21:12:09Z

Do you know mvn hpi:run? It is very handy to interactively try out the latest changes during development.

Thank you for this advice, I was able to reproduce the original issue with this. And it seems to be resolved with this pull request.
Because the bitbucket server I usually use is behind a corporate firewall I was not able to actually push a status yet, because some other configurations need to be done. But using the release/1.x state with buildName: '', buildStatus: '' I get the exact same output I get with this PR branch without these parameters specified (and I get the initial missing parameters problem of course when I do not set the params on the release/1.x branch).

darxriggs · 2020-02-13T20:21:59Z

src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java

-        return considerUnstableAsSuccess;
+    @DataBoundSetter
+    public void setStashServerBaseUrl(String stashServerBaseUrl) {
+        this.stashServerBaseUrl = stashServerBaseUrl != null && stashServerBaseUrl.endsWith("/")


StringUtils#stripEnd could be used.

src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java

westarne · 2020-02-13T20:50:41Z

Implemented the proposed optimizations

darxriggs · 2020-02-13T21:01:49Z

src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java

-        this.prependParentProjectKey = prependParentProjectKey;
-        this.disableInprogressNotification = disableInprogressNotification;
-        this.considerUnstableAsSuccess = considerUnstableAsSuccess;
+    StashNotifier(


I am thinking if the number of constructors can be reduced even more? Is this one really required?

I'm not sure about that, and neither am I that the test cases existing as well as the ones I can do locally cover all potential use cases. So I've kept it to be on the safe side.

But this is a new one and another one has been removed.

In other plugins it's often handled the way to keep all old constructors for backward compatibility and annotate them with @Deprecated. That's something the maintainers should decide.

Oh you're right. Then I'm not totally sure, what the constructor was meant for. I think it's just the old DataBoundConstructor without all of the optional parameters. So I could just move the logic in the no-argument constructor.

Was some time out of the country and noticed that I did not push the change before. Done that now.

coxalass · 2021-06-09T10:05:01Z

Hello,

I guess this might also fix the following issue : #266

Is there anything preventing this PR from being merged ?

Kind regard,

jetersen

This is usually best practice for Jenkins plugins and follows JCasC recommendations as well.

scaytrase · 2021-12-19T16:44:08Z

At the moment I think I can accept this PR, but this should be updated

scaytrase · 2022-01-22T01:58:07Z

@westarne a lot of compilation errors

westarne · 2022-01-22T02:04:15Z

@westarne a lot of compilation errors

never merge on the git website, noted. Was just missing a } as far as I can tell. Added that

scaytrase

LGTM 👍

scaytrase · 2022-01-22T15:13:17Z

Thanks @westarne . I'll try try to release this as soon as I can

scaytrase · 2022-01-22T21:40:02Z

https://github.com/jenkinsci/stashnotifier-plugin/releases/tag/1.25

Implement setters instead of constructor for optional parameters

d34dbce

Fixes #245

proski reviewed Dec 5, 2019

View reviewed changes

src/main/resources/org/jenkinsci/plugins/stashNotifier/StashNotifier/config.jelly Show resolved Hide resolved

Merge branch 'release/1.x' into feature/optional-parameters

fc16285

darxriggs reviewed Feb 13, 2020

View reviewed changes

src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java Show resolved Hide resolved

Optimize setters

615d351

darxriggs reviewed Feb 13, 2020

View reviewed changes

Remove unnecessary constructor

ed5bde5

jetersen approved these changes Dec 18, 2021

View reviewed changes

Merge branch 'release/1.x' into feature/optional-parameters

e3c5982

Fix missing brace

a6e1355

scaytrase approved these changes Jan 22, 2022

View reviewed changes

scaytrase merged commit 5d1ba63 into jenkinsci:release/1.x Jan 22, 2022

westarne deleted the feature/optional-parameters branch January 22, 2022 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement setters instead of constructor for optional parameters #246

Implement setters instead of constructor for optional parameters #246

westarne commented Oct 2, 2019 •

edited

Loading

westarne commented Oct 4, 2019

scaytrase commented Oct 4, 2019

westarne commented Oct 4, 2019

scaytrase commented Oct 4, 2019

proski commented Dec 3, 2019

westarne commented Dec 3, 2019 •

edited

Loading

scaytrase commented Dec 3, 2019

darxriggs commented Jan 6, 2020

scaytrase commented Jan 6, 2020

westarne commented Jan 6, 2020

darxriggs commented Jan 6, 2020

westarne commented Jan 6, 2020 •

edited

Loading

darxriggs Feb 13, 2020

westarne commented Feb 13, 2020

darxriggs Feb 13, 2020 •

edited

Loading

westarne Feb 13, 2020

darxriggs Feb 13, 2020

westarne Feb 13, 2020

westarne Apr 2, 2020 •

edited

Loading

coxalass commented Jun 9, 2021

jetersen left a comment

scaytrase commented Dec 19, 2021

scaytrase commented Jan 22, 2022

westarne commented Jan 22, 2022

scaytrase left a comment

scaytrase commented Jan 22, 2022

scaytrase commented Jan 22, 2022

Implement setters instead of constructor for optional parameters #246

Implement setters instead of constructor for optional parameters #246

Conversation

westarne commented Oct 2, 2019 • edited Loading

westarne commented Oct 4, 2019

scaytrase commented Oct 4, 2019

westarne commented Oct 4, 2019

scaytrase commented Oct 4, 2019

proski commented Dec 3, 2019

westarne commented Dec 3, 2019 • edited Loading

scaytrase commented Dec 3, 2019

darxriggs commented Jan 6, 2020

scaytrase commented Jan 6, 2020

westarne commented Jan 6, 2020

darxriggs commented Jan 6, 2020

westarne commented Jan 6, 2020 • edited Loading

darxriggs Feb 13, 2020

Choose a reason for hiding this comment

westarne commented Feb 13, 2020

darxriggs Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

westarne Feb 13, 2020

Choose a reason for hiding this comment

darxriggs Feb 13, 2020

Choose a reason for hiding this comment

westarne Feb 13, 2020

Choose a reason for hiding this comment

westarne Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

coxalass commented Jun 9, 2021

jetersen left a comment

Choose a reason for hiding this comment

scaytrase commented Dec 19, 2021

scaytrase commented Jan 22, 2022

westarne commented Jan 22, 2022

scaytrase left a comment

Choose a reason for hiding this comment

scaytrase commented Jan 22, 2022

scaytrase commented Jan 22, 2022

westarne commented Oct 2, 2019 •

edited

Loading

westarne commented Dec 3, 2019 •

edited

Loading

westarne commented Jan 6, 2020 •

edited

Loading

darxriggs Feb 13, 2020 •

edited

Loading

westarne Apr 2, 2020 •

edited

Loading