-
Notifications
You must be signed in to change notification settings - Fork 369
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
[ci-app] Add User Provided Git Metadata #1662
[ci-app] Add User Provided Git Metadata #1662
Conversation
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.
LGTM left some minor suggestions
Co-authored-by: Adrián López Calvo <adrian.lopezcalvo@datadoghq.com>
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.
Overall seems fine, but I think we should migrate some constants to follow convention.
lib/datadog/ci/ext/environment.rb
Outdated
Datadog::Ext::Git::TAG_COMMIT_COMMITTER_NAME => env['DD_GIT_COMMIT_COMMITTER_NAME'], | ||
Datadog::Ext::Git::TAG_COMMIT_COMMITTER_EMAIL => env['DD_GIT_COMMIT_COMMITTER_EMAIL'], | ||
Datadog::Ext::Git::TAG_COMMIT_COMMITTER_DATE => env['DD_GIT_COMMIT_COMMITTER_DATE'] | ||
}.reject { |_, v| v.nil? || v.empty? } |
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 would recommend using strip
on the values first, as sometimes users will introduce whitespace characters that will make the string "not empty", e.g.:
2.5.1 :002 > ' '.empty?
=> false
lib/datadog/ci/ext/environment.rb
Outdated
@@ -324,6 +327,22 @@ def extract_bitrise(env) | |||
} | |||
end | |||
|
|||
def extract_user_defined_git(env) | |||
{ | |||
Datadog::Ext::Git::TAG_REPOSITORY_URL => env['DD_GIT_REPOSITORY_URL'], |
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.
All these strings should be frozen, migrated to Datadog::Ext::Git
and prefixed with ENV_
e.g. 'DD_GIT_REPOSITORY_URL'
=> Datadog::Ext::Git::ENV_REPOSITORY_URL = 'DD_GIT_REPOSITORY_URL'.freeze
is_expected.to eq( | ||
{ | ||
'ci.workspace_path' => "#{Dir.pwd}/spec/datadog/ci/ext/fixtures/git", | ||
'git.branch' => 'my-branch', |
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.
Minor suggestion, but you could derive these values from the env
above e.g. 'git.branch' => env['DD_GIT_BRANCH']
Motivation
Provide a way for users to provide
git
metadata when other methods fail.How
git
metadata.Note: I don't understand what's happening with
sorbet
's job. If I run the typecheck in master the same errors show, so I'm not sure if they're related to my changes. Also, the files with errors seem not to be the ones I changed?