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.

Sign up for GitHub

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

Jump to bottom

Allow setting Git information via environment variables #252

Merged
merged 7 commits into from
Oct 25, 2021
Merged

Allow setting Git information via environment variables #252

merged 7 commits into from
Oct 25, 2021

Conversation

drodriguezhdez
Copy link
Collaborator

@drodriguezhdez drodriguezhdez commented Oct 21, 2021 •
edited
Loading

Requirements for Contributing to this repository

  • Fill out the template below. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • The pull request must only fix one issue at the time.
  • The pull request must update the test suite to demonstrate the changed functionality.
  • After you create the pull request, all status checks must be pass before a maintainer reviews your contribution. For more details, please see CONTRIBUTING.

What does this PR do?

This PR adds the logic to allow the user to set Git information via environment variables.

These variables are optional, but if they are set, they take precedence over the same git information that Jenkins could provide.

DD_GIT_REPOSITORY_URL : The url of the repository where the code is stored 
DD_GIT_DEFAULT_BRANCH: The default branch of the repository. To be only used on pipelines that support custom environment variables (currently only Jenkins). Not supported on tests
DD_GIT_BRANCH : The branch where this commit belongs to
DD_GIT_TAG: The tag of the commit, if it has one
DD_GIT_COMMIT_SHA :  The commit hash of the current code
DD_GIT_COMMIT_MESSAGE :  The commit message
DD_GIT_COMMIT_AUTHOR_NAME :  The commit author name
DD_GIT_COMMIT_AUTHOR_EMAIL :  The commit author email
DD_GIT_COMMIT_AUTHOR_DATE :  The commit author date (ISO 8601)
DD_GIT_COMMIT_COMMITTER_NAME :  The commit committer name
DD_GIT_COMMIT_COMMITTER_EMAIL :  The commit committer email
DD_GIT_COMMIT_COMMITTER_DATE :  The commit committer date (ISO 8601)

Description of the Change

Alternate Designs

Possible Drawbacks

Verification Process

Additional Notes

Release Notes

Review checklist (to be filled by reviewers)

  • Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
  • PR should not have do-not-merge/ label attached.
  • If Applicable, issue must have kind/ and severity/ labels attached at least.

@drodriguezhdez drodriguezhdez marked this pull request as ready for review October 22, 2021 07:56
drodriguezhdez
drodriguezhdez commented Oct 22, 2021
View reviewed changes
src/main/java/org/datadog/jenkins/plugins/datadog/DatadogUtilities.java
public static String getGitRepositoryUrl(final EnvVars envVars) {
return getGitRepositoryUrl(envVars.entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)));
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to GitUtils class with the logic to select the correct env var (either user supplied env var or jenkins env var)

edrevo
edrevo reviewed Oct 25, 2021
View reviewed changes
src/main/java/org/datadog/jenkins/plugins/datadog/DatadogUtilities.java Outdated
* @param envVars
* @return map
*/
public static Map<String, String> toMap(EnvVars envVars) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EnvVars already implements Map<String, String> according to https://javadoc.jenkins-ci.org/hudson/EnvVars.html

src/main/java/org/datadog/jenkins/plugins/datadog/model/BuildData.java Outdated
Comment on lines 405 to 409
// These are the most common values the user will provide.
return envVars.get(DD_GIT_REPOSITORY_URL) != null ||
envVars.get(DD_GIT_BRANCH) != null ||
envVars.get(DD_GIT_TAG) != null ||
envVars.get(DD_GIT_COMMIT_SHA) != null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this check is super consistent. If we are providing more env vars, I guess we should check all of them. For example, if the user is providing author information to adjust their billing I don't see why we should be ignoring it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to use all the user-supplied env vars.

src/main/java/org/datadog/jenkins/plugins/datadog/model/BuildData.java
setBranch(envVars.get("GIT_BRANCH"));
setGitUrl(DatadogUtilities.getGitRepositoryUrl(envVars));
setGitCommit(envVars.get("GIT_COMMIT"));
if (isGit(envVars)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is isGit only called in BuildData but not in DatadogTracePipelineLogic?

src/main/java/org/datadog/jenkins/plugins/datadog/model/BuildData.java Outdated
Comment on lines 354 to 356
if(getGitMessage("").isEmpty()){
this.gitMessage = gitCommitAction.getMessage();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't you using the setters here?

Suggested change
if(getGitMessage("").isEmpty()){
this.gitMessage = gitCommitAction.getMessage();
}
setGitMessage(getGitMessage(gitCommitAction.getMessage());

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No specific reason. Changed to setters 👌🏼

@drodriguezhdez
Copy link
Collaborator Author

Why is isGit only called in BuildData but not in DatadogTracePipelineLogic?

This method is used in BuildData because the branch attribute is being used in both CVS and GIT pipelines fashion. (inherited from non-CIAPP features).

The logic of the DatadogTracePipelineLogic only takes into account the GIT pipeline fashion.

@drodriguezhdez drodriguezhdez merged commit 224e38d into master Oct 25, 2021
@drodriguezhdez drodriguezhdez deleted the drodriguezhdez/git_env_vars branch October 25, 2021 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edrevo edrevo edrevo approved these changes

@FlorianVeaux FlorianVeaux Awaiting requested review from FlorianVeaux

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@drodriguezhdez @edrevo