Allow setting Git information via environment variables by drodriguezhdez · Pull Request #252 · jenkinsci/datadog-plugin

drodriguezhdez · 2021-10-21T16:02:22Z

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 · 2021-10-22T07:35:16Z

-    public static String getGitRepositoryUrl(final EnvVars envVars) {
-        return getGitRepositoryUrl(envVars.entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)));
-    }
-


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

edrevo · 2021-10-25T07:03:59Z

+     * @param envVars
+     * @return map
+     */
+    public static Map<String, String> toMap(EnvVars envVars) {


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

edrevo · 2021-10-25T07:09:08Z

+        // 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;


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.

Changed to use all the user-supplied env vars.

edrevo · 2021-10-25T07:10:40Z

-            setBranch(envVars.get("GIT_BRANCH"));
-            setGitUrl(DatadogUtilities.getGitRepositoryUrl(envVars));
-            setGitCommit(envVars.get("GIT_COMMIT"));
+        if (isGit(envVars)) {


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

edrevo · 2021-10-25T07:23:02Z

+            if(getGitMessage("").isEmpty()){
+                this.gitMessage = gitCommitAction.getMessage();
+            }


Why aren't you using the setters here?

Suggested change

if(getGitMessage("").isEmpty()){

this.gitMessage = gitCommitAction.getMessage();

}

setGitMessage(getGitMessage(gitCommitAction.getMessage());

No specific reason. Changed to setters 👌🏼

drodriguezhdez · 2021-10-25T08:28:48Z

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.

Daniel Rodriguez Hernandez added 5 commits October 21, 2021 16:46

Support user supplied git environment variables

e24ab0d

User provided Git environment variables

d0d782a

Refactor resolve git functions

5b2e518

Remove unnecesary constant

1404db7

Add comments

43be389

drodriguezhdez marked this pull request as ready for review October 22, 2021 07:56

drodriguezhdez requested review from FlorianVeaux and edrevo October 22, 2021 07:56

drodriguezhdez commented Oct 22, 2021

View reviewed changes

Use correct function

a98fbdb

drodriguezhdez force-pushed the drodriguezhdez/git_env_vars branch from dc693d1 to a98fbdb Compare October 22, 2021 08:52

edrevo reviewed Oct 25, 2021

View reviewed changes

drodriguezhdez commented Oct 25, 2021

View reviewed changes

Comment thread src/main/java/org/datadog/jenkins/plugins/datadog/model/BuildData.java Outdated

Apply PR feedback

3014a9f

edrevo approved these changes Oct 25, 2021

View reviewed changes

drodriguezhdez merged commit 224e38d into master Oct 25, 2021

drodriguezhdez deleted the drodriguezhdez/git_env_vars branch October 25, 2021 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow setting Git information via environment variables#252

Allow setting Git information via environment variables#252
drodriguezhdez merged 7 commits into
masterfrom
drodriguezhdez/git_env_vars

drodriguezhdez commented Oct 21, 2021 •

edited

Loading

Uh oh!

drodriguezhdez Oct 22, 2021

Uh oh!

edrevo Oct 25, 2021

Uh oh!

edrevo Oct 25, 2021

Uh oh!

drodriguezhdez Oct 25, 2021

Uh oh!

edrevo Oct 25, 2021

Uh oh!

edrevo Oct 25, 2021

Uh oh!

drodriguezhdez Oct 25, 2021

Uh oh!

Uh oh!

drodriguezhdez commented Oct 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

drodriguezhdez commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Requirements for Contributing to this repository

What does this PR do?

Description of the Change

Alternate Designs

Possible Drawbacks

Verification Process

Additional Notes

Release Notes

Review checklist (to be filled by reviewers)

Uh oh!

drodriguezhdez Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

edrevo Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

edrevo Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

drodriguezhdez Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

edrevo Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

edrevo Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

drodriguezhdez Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

drodriguezhdez commented Oct 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

drodriguezhdez commented Oct 21, 2021 •

edited

Loading