Added environment variables for MR comment event #127
Conversation
eugenelesnov
force-pushed
the
trigger_comment_environment_variable
branch
from
2683d9a to
e618502
Compare
|
@eugenelesnov Thanks! Your solution lgtm ! |
|
@MikeSafonov good 😎 |
LinuxSuRen
left a comment
LinuxSuRen
left a comment
There was a problem hiding this comment.
Thanks for your contribution. I left some comments. Please take a look at it.
| * @param commentUrl the URL for the GitLab comment | ||
| */ | ||
| public GitLabMergeRequestCommentCause(String commentUrl) { | ||
| public GitLabMergeRequestCommentCause(String commentUrl, NoteEvent noteEvent) { |
There was a problem hiding this comment.
I prefer to add a new constructor instead of changing the existing one. Because you cannot sure there are no others who might need that.
There was a problem hiding this comment.
Good point, sir
| this.variables.put("GITLAB_REPO_VISIBILITY_LEVEL", defaultVisibilityString(noteEvent.getRepository().getVisibility_level())); | ||
| this.variables.put("GITLAB_REQUEST_URL", defaultString(noteEvent.getRequestUrl())); | ||
| this.variables.put("GITLAB_REQUEST_STRING", defaultString(noteEvent.getRequestQueryString())); | ||
| this.variables.put("GITLAB_REQUEST_TOKEN", defaultString(noteEvent.getRequestSecretToken())); |
There was a problem hiding this comment.
I'm not sure if it's safe here.
| this.variables.put("GITLAB_COMMENT_TRIGGER", defaultString(noteEvent.getObjectAttributes().getNote())); | ||
| this.variables.put("GITLAB_USER_ID", defaultIntString(noteEvent.getUser().getId())); | ||
| this.variables.put("GITLAB_PROJECT_ID", defaultIntString(noteEvent.getProjectId())); | ||
| this.variables.put("GITLAB_PROJECT_ID_2", defaultIntString(noteEvent.getProject().getId())); |
There was a problem hiding this comment.
What's the difference between GITLAB_PROJECT_ID and GITLAB_PROJECT_ID_2?
|
|
||
| private final Map<String, String> variables = new HashMap<>(); | ||
|
|
||
| public GitLabMergeRequestNoteData(NoteEvent noteEvent) { |
There was a problem hiding this comment.
It's great to have some useful environment variables. But I have two questions about it.
- How users can know the variable name and its meaning
- Do we need so many variables? Or they're all?
There was a problem hiding this comment.
Answering all comments above here. Most of variables were copied from another classes from io.jenkins.plugins.gitlabbranchsource.Cause package. My bad, meh.
For sure, GITLAB_COMMENT_TRIGGER is enough..
eugenelesnov
force-pushed
the
trigger_comment_environment_variable
branch
from
e618502 to
579411f
Compare
eugenelesnov
force-pushed
the
trigger_comment_environment_variable
branch
from
579411f to
e2a9688
Compare
LinuxSuRen
left a comment
LinuxSuRen
left a comment
There was a problem hiding this comment.
LGTM
Let's wait for some potential comments from other reviewers.
4 participants
resolves #123
also relevant to JENKINS-60282
Added environment variables to access NoteEvent (comment on MR) data.