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

Partial implementation for JENKINS-26103 #274

Merged
merged 6 commits into from Mar 16, 2016

Conversation

Projects
None yet
3 participants
@teemumurtola
Copy link
Contributor

commented Feb 9, 2016

Implement a basic workflow step for influencing the review posted back to Gerrit at the end of the build. This makes it possible to customize at least some aspects of the post, as can be done with environment variables in a freestyle project (see JENKINS-32692). The same step could be extended in the future to post intermediate reviews and/or to other changes, by adding optional parameters such as 'postImmediately' and 'refspec'.

Dependency on workflow plugin is optional.

teemumurtola added some commits Feb 8, 2016

Workflow step for setting unsuccessful message
Add a workflow step that allows setting the unsuccessful message from a
workflow build.  config.jelly is currently missing.

Dependency to the workflow plugin is optional.

Implements a limited part of [JENKINS-26103].
Allow setting custom url in workflow
Extend the gerritReview workflow step to also allow setting the URL to
post.  Use the existing mechanism (BuildMemory) for storing the value
while the build is running.

Implements a limited part of [JENKINS-26103].
@rsandell

This comment has been minimized.

Copy link
Member

commented Feb 9, 2016

This is great! Thanks for implementing it.
I'll dive into the details as soon as I'm able, I need to remember how the customUrl etc. is handled today to understand if there might be any corner cases with your implementation :)

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Feb 9, 2016

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

@teemumurtola

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2016

Thanks for the quick response. Some comments on my own implementation:

  • I wasn't sure about what version of the workflow plugin to put as a dependency, so I just used the same that was already there for the tests.
  • I haven't tested this with a real Gerrit instance, only with the tests added in the commits.
  • I tested that the Snippet Generator in the workflow build configuration page works reasonably, but I'm not sure whether the handling of null/empty values passed to the step are the best possible. But already in this form it should be quite usable.
Entry entry = pb.getEntry(r.getParent());

if (entry != null) {
logger.info("Recording custom URL for {}: {}", event, customUrl);

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 22, 2016

Member

lower this to debug or trace and you can fix around line 422 while you're at it ;)

This comment has been minimized.

Copy link
@teemumurtola

teemumurtola Feb 22, 2016

Author Contributor

Will do, once the discussion for the other comments is ok.

}
String unsuccessfulMessage = step.getUnsuccessfulMessage();
if (unsuccessfulMessage != null) {
listener.setBuildFailureMessage(build, unsuccessfulMessage);

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 22, 2016

Member

A build failure and an unsuccessful build are different in some sense. But this naming is consistent with the existing trigger config so I guess we should keep it like that. Just a bit unfortunate that the naming was scewed from the beginning.

This comment has been minimized.

Copy link
@teemumurtola

teemumurtola Feb 22, 2016

Author Contributor

Yes, there is some inconsistent naming here. If you want, I can rename setBuildFailureMessage() (and BuildMemory.setEntryFailureMessage(), and possibly also obtainFailureMessage()), so that they all refer to unsuccessfulMessage. That's how they all work, anyways, so I think the user-visible naming of "unsuccessful message" is reasonable. I'll also check (and fix if broken) the handling of the unsuccessful message set here in case the build actually succeeds.

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 23, 2016

Member

Yes "unsuccessful message" is the more "correct" term I agree. But if you choose to change all that you would need to keep the original methods as deprecated and route around to keep binary compatibility. That's why I didn't suggest to do the renaming because it would be a bit of work.

If you still want to do it it should be done in a separate commit so that we don't mix things up too much ;) Except for maybe the newly added method in this change.

This comment has been minimized.

Copy link
@teemumurtola

teemumurtola Feb 23, 2016

Author Contributor

Ok, I can rename the method added here (if you think it would improve things), but leave the others intact. I assume that obtainFailureMessage() could be renamed without worries about compatibility (since it is private), but that still leaves the one existing method with different naming. I can't judge whether any other plugin could or should really depend on it.

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 24, 2016

Member

Yes obtainFailureMessage() can be renamed without any problems, for the public methods we should make an effort to stay backwards compatible, e.g. keep the original method deprecated and just call the new method from it, just to be safe.

But I would rather that we are consistent and a bit wrong than inconsistent and a bit right :) Meaning you should only rename this new method if you feel you have the time to make all the corrections to stay consistent.
I would be very happy if you have time to make those corrections, but I don't expect you to do that work if you don't want to.
So only do the rename (of the new method) if you feel you have time/want to do the other rename work as well. Unfortunately I don't have much spare time to do it myself, so it's better to leave it consistent and a bit wrong in that case :) .

This comment has been minimized.

Copy link
@teemumurtola

teemumurtola Feb 26, 2016

Author Contributor

Ok, I'll do all the renaming, and leave the one old public method as deprecated.

@@ -103,6 +103,12 @@
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-step-api</artifactId>
<version>1.4</version>

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 22, 2016

Member

imho we should target 1.11 or workflow-plugin, but if this works then let's keep it like that.


@Override
public String getFunctionName() {
return "gerritReview";

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 22, 2016

Member

I don't like the name, mostly because it can imply that the review will be sent to Gerrit as the step is executed (a feature I've been planning to implement for a while) It should have a name that implies properties being set rather than some immediate action. setGerritReview maybe?

This comment has been minimized.

Copy link
@teemumurtola

teemumurtola Feb 22, 2016

Author Contributor

I thought about the alternatives a bit, but I do not have a strong preference, so I can change this to something else as well. gerritSetReview or setGerritReview are reasonable alternatives. The reason why I ended up with this name is that I thought that in the long run, all the below four alternatives could be reasonably useful for someone:

  • Change the final comment/vote for the change that triggered this build, as done here.
  • Post comments/votes to other changes, once this build and/or the triggered builds finish.
  • Post immediately to the change that triggered this build.
  • Post immediately to some other change.

And for all of these, the parameters influencing what to post would be essentially identical. So the same step could do all of them, with just a separate parameter (e.g., postImmediately: true) deciding how to post, and the name without "set" is suitable for all. But if you prefer, I'm fine with having separate names for these different use cases. Because of backwards-compatibility concerns, it can be difficult to change from one approach to another, though.

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 23, 2016

Member

I think the "postImmediately" implementation would be very different from what is implemented now and should be in a separate step implementation for easier maintainability and lower complexity, and so would require it's own name. And gerritReview is more suited for that.

This comment has been minimized.

Copy link
@teemumurtola

teemumurtola Feb 23, 2016

Author Contributor

I agree that the run() implementation would be very different, probably for all four cases. But that is relatively easy to dispatch to different functions with just a few if's. The other side is that if it is in different steps, it may end up duplicating a lot of the setters/getters for the step parameters (at least most of which would/should be identical for all the steps to keep things consistent), and the jelly/html files for them. I don't know enough of how the jelly side works to know whether there is some way to avoid that. But that said, I'm fine with splitting this, and I agree that if we split it, we should rename the current implementation.

Another, minor concern that I have in the back of my head is that if there are a lot of plugins each contributing a lot of different workflow steps, at least the current workflow snippet generator might not be very good for finding them. But that might not be a concern for this change...

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 24, 2016

Member

The duplication of set/get and jellies can be solved with inheritance.

A lot of plugins contributing a lot of steps would be a very nice problem to have :)

@rsandell

This comment has been minimized.

Copy link
Member

commented Feb 22, 2016

The build failed because of a timeout, we get another chance if/when you upload changes for my comments ;)

@teemumurtola

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2016

On a general level, what's the desired workflow for updating the pull request (haven't worked much with github before)? Should I just push additional commits to the branch, or edit the existing commits for cases where applicable? I'm fine with either; after working a long time with Gerrit, editing existing commits comes quite naturally. ;) Some of the changes (like renaming the existing methods) I'll anyways do as separate commits, but I hope it is still OK to include them in this same pull request?

@rsandell

This comment has been minimized.

Copy link
Member

commented Feb 26, 2016

The preferred way, as it seems in the Jenkins community at least, is to just continue adding commits to the branch (no commit --amend). The argument is that it is then easier to follow the thought process of the developer. And there is always a merge commit of the pull request when it lands on master (merged with --no-ff) so it's easy to revert all those, potentially, 100 commits. I usually consider any commits between pull request merges as just such details.

It used to be that GitHub didn't handle amended commits very well when displaying line comments, but that has been fixed for a while, and since this particular plugin is usually contibuted to by Gerrit users I don't mind amended commits, but I don't normally do them myself unless I feel I need to hide some embarrassing change ;)

So on this particular plugin, do what you feel comfortable with, but in most other Jenkins repos the normal is to just add commits.

teemumurtola added some commits Feb 26, 2016

Use "unsuccessful message" more consistently
Rename a few methods from "failure message" to "unsuccessful message"
for better consistency.  One older public method is still kept as
deprecated and forwards calls to the new method.
Similar renaming also in comments and tests.
Logging adjustments
- Reduce some logging levels.
- Adjust log message to match both workflow and freestyle job behavior.
Rename gerritReview workflow step to setGerritReview
This is a better name for the current functionality, and it is probably
better to add additional functionality as separate steps instead of
making a single step that does everything.
@teemumurtola

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2016

Sorry for some delays, I've been a bit busy with other stuff lately. The updated pull request should now address all the discussed points:

  • The workflow step is now named setGerritReview.
  • The logging levels have been adjusted (as well as one log entry that was no longer accurate for workflow jobs).
  • I renamed all relevant instances of "failure message" to "unsuccessful message" when the latter was meant. In addition to the entries discussed above, I found a few in the tests that I also renamed for consistency. The old public method is still left as deprecated.
  • I added a test for setting an unsuccessful message in a successful workflow, and it worked, so there was no need to do other changes related to this.

rsandell added a commit that referenced this pull request Mar 16, 2016

Merge pull request #274 from teemumurtola/workflow-steps
Partial implementation for JENKINS-26103

@rsandell rsandell merged commit b7ab589 into jenkinsci:master Mar 16, 2016

1 check passed

Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.