-
Notifications
You must be signed in to change notification settings - Fork 25
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-63938] Add storage of commentBody in GitHubPullRequestCommentCause #25
[JENKINS-63938] Add storage of commentBody in GitHubPullRequestCommentCause #25
Conversation
|
||
/** | ||
* Constructor. | ||
* @param commentUrl the URL for the GitHub comment | ||
*/ | ||
public GitHubPullRequestCommentCause(String commentUrl) { | ||
this.commentUrl = commentUrl; | ||
this.commentBody = "Unknown"; |
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 feel that this is not correct. it really should be an empty string. Otherwise how could someone tell the difference between "Unknown" as a comment and this default case?
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.
Alternatively, you could take out this constructor since I believe it's not used anymore, although people outside of the plugin may have depended on it. However, they can also just update their usage if they are using internal classes, so I think I'm leaning towards just removing this version of the constructor.
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.
Thank you for getting the build working again. I've gone ahead and removed the old constructor. I wasn't sure at first how backwards compatible things needed to be, but agree that it's completely unnecessary now internally.
Ok, finally got the build working again in #28, but looking at the PR a little more in depth I noticed something and commented on it. Please get that fixed and we can get this merged in and released. |
…ent body in the GitHubPullRequestCommentCause class
…equestCommentCause
ed4f904
to
2610b22
Compare
@bluesliverx Thank you for getting to this, and for getting the build fixed. I have gone ahead and rebased off your recent changes that are in master and addressed your comment. |
This PR is to address the first part of the issue presented by #22 by adding the storage of the GitHub
comment body to the resulting build cause. With this, one can now easily retreive the comment body
that triggered the build to start so that it can be parsed during the build. Further work can
be done on top of this PR to store this information as an environment variable on the Jenkins job
itself, but this alone helps to remove the need to create an API call out to GitHub for information
that was already sent in the initial webhook.