-
Notifications
You must be signed in to change notification settings - Fork 610
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
Implement the DSL for commentFilePath to fix #514 #709
Conversation
Changelog Swapped two lines back that were swapped by accident. Constructor use of commentFilePath Add the GhprbCommentFile extension to the list of extensions
If the comment file was completely empty it was still read, surrounded with a header and a footer, and posted as a comment on the pull request. This is just noise IMHO. This commit makes it so that the message stays empty if the file was empty.
@samrocketman, had some issues with the other PR (#637). This one has the collection of changes and is up to date with master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code-wise looks ok to me. I'm not sure how to test this.
Maybe make a seed job and have it try to process a dsl with this option on? |
I don't have time to work on testing this at the moment since I'm preparing for Jenkins World next week where I'm presenting. |
Oh, looking at your test infrastructure, it doesn't use groovy, which dsl requires, so unless testing can support groovy, that wouldn't be a good way to test. Looks like none of the other DSL gets tested. Just looking at the code of this merge, it looks reasonable. |
README.md
Outdated
@@ -144,6 +144,7 @@ job('upstreamJob') { | |||
permitAll() | |||
autoCloseFailedPullRequests() | |||
displayBuildErrorsOnDownstreamBuilds() | |||
commentFilePath("relative/path/to/file") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, isn't this changed now according to this: #637 (comment)
Yeah, the job dsl testing happens manually at this time. |
Merging is blocked I pushed the README update requested by @Kentalot. |
I'm not working on this (or any open source projects) until after Jenkins World next week. |
@samrocketman Any updates on this one? 👍 |
Not available for coding. Traveling on vacation at the moment. |
I will be back home Oct 17th. If you see no action in the week following that date feel free to ping me again to remind me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote for merging.
@bjoernhaeuser feel free to merge LGTM |
@samrocketman thanks for approving. |
Awesome now that this is merged, any way we could get #682 merged? :) |
@bjoernhaeuser I tested this on my DSL job and it seems to not work as it somehow does not seem to find commentFilePath method that takes in a closure. Not sure what's happening. Your #664 update also fails with a similar error. I checked to make sure i have the latest ghprb plugin version (1.42.0). commitStatus and buildStatus extensions work just fine. I don't see anything wrong with your code and the only difference I could see between the working ones (commitStatus and buildStatus) and the non-working ones (commentFilePath and cancelBuildsOnUpdate) is the latter ones have their methods public. I doubt that's the issue, but I couldn't see any other difference. Here's the error:
and the code snippet:
|
Implement the DSL for commentFilePath to fix jenkinsci#514
No description provided.