-
Notifications
You must be signed in to change notification settings - Fork 375
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:Read metadata from local git repository #1561
Conversation
it_behaves_like 'matching git tags', 'gitdir_with_commit', { | ||
'ci.workspace_path' => "#{Dir.pwd}/spec/datadog/ci/ext/fixtures/git", | ||
'git.branch' => 'master', | ||
'git.commit.author.date' => '2011-02-16T13:00:00+00:00', |
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.
@juan-fernandez Is this date format what we expect for the CI app?
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.
yeah I think this should work. The backend is pretty flexible on this I think. I'm sending this format Fri Jun 18 16:14:22 2021 +0200
and it works too. Easiest way would be to send some data to CI App from this PR: is that possible?
@@ -312,6 +317,107 @@ def extract_bitrise(env) | |||
} | |||
end | |||
|
|||
def git_commit_users |
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'll refactor this whole file into separate "extractors" in a follow up PR.
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.
🙏
Codecov Report
@@ Coverage Diff @@
## master #1561 +/- ##
=======================================
Coverage 98.24% 98.24%
=======================================
Files 885 885
Lines 42592 42674 +82
=======================================
+ Hits 41843 41926 +83
+ Misses 749 748 -1
Continue to review full report at Codecov.
|
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.
looking good!
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.
Left a few small notes, but overall it LGTM 👍
@@ -312,6 +317,107 @@ def extract_bitrise(env) | |||
} | |||
end | |||
|
|||
def git_commit_users |
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.
🙏
lib/datadog/ci/ext/environment.rb
Outdated
# Because we can't get a reliable UTC time from all recent versions of git | ||
# We have to rely on converting the date to UTC ourselves. |
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: Consider moving this right next to the date conversion, rather than just having it randomly here
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 added it here because it's where the git command that returns the date format is: git show -s --format='%an\t%ae\t%at\t%cn\t%ce\t%ct'
.
But if you are confused by it, then I'll be too in a few months, so I'm moving it to the date formatting place.
lib/datadog/ci/ext/environment.rb
Outdated
def git_base_directory | ||
git_dir = exec_git_command('git rev-parse --git-dir') | ||
return unless git_dir | ||
|
||
# The result will point to the '.git' directory. | ||
# We actually want the base source directory. | ||
File.expand_path('..', git_dir) |
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.
Not sure if we want to care about it, but note that this is not correct in the presence of git worktrees.
I keep a single checkout of dd-trace-rb but multiple worktrees (to be able to keep several branches open at once). For those, this doesn't look correct:
ivo.anjo@macieira:~/datadog/dd-trace-rb$ cat ../base-directory.rb
puts `git rev-parse --git-dir`.strip
puts File.expand_path('..', `git rev-parse --git-dir`.strip)
ivo.anjo@macieira:~/datadog/dd-trace-rb$ ruby ../base-directory.rb
.git
/Users/ivo.anjo/datadog/dd-trace-rb
ivo.anjo@macieira:~/datadog/dd-trace-rb$ cd ../second-dd-trace-rb/
ivo.anjo@macieira:~/datadog/second-dd-trace-rb$ ruby ../base-directory.rb
/Users/ivo.anjo/datadog/dd-trace-rb/.git/worktrees/second-dd-trace-rb
/Users/ivo.anjo/datadog/dd-trace-rb/.git/worktrees
Not we may not want to care about this for CI, but just thought I'd point out (since I was curious myself)
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 changed it so that it works with worktrees now.
Using git rev-parse --show-toplevel
seems more correct in any case.
I tried to add a test case for worktrees using the GIT_DIR
environment variable, like I'm using for git testing here, but couldn't get it to work.
I tried different combinations of environment variables, but couldn't get: a worktree that's a spin-off of GIT_DIR=spec/datadog/ci/ext/fixtures/git/gitdir_with_commit
that works correctly inside a spec running inside a dd-trace-rb
directory.
For now, I'm ok with this case, and I like that git rev-parse --show-toplevel
doesn't perform any ../
path shenanigans.
lib/datadog/ci/ext/environment.rb
Outdated
Datadog::Ext::Git::TAG_COMMIT_AUTHOR_NAME => (author[:name] if author), | ||
Datadog::Ext::Git::TAG_COMMIT_AUTHOR_EMAIL => (author[:email] if author), | ||
Datadog::Ext::Git::TAG_COMMIT_AUTHOR_DATE => (author[:date] if author), | ||
Datadog::Ext::Git::TAG_COMMIT_COMMITTER_NAME => (committer[:name] if committer), | ||
Datadog::Ext::Git::TAG_COMMIT_COMMITTER_EMAIL => (committer[:email] if committer), | ||
Datadog::Ext::Git::TAG_COMMIT_COMMITTER_DATE => (committer[:date] if committer) |
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: This may be simplified a bit if rather than an optional array of optional hashes, we got just a flat hash from git_commit_users
with {author_name: ..., author_email: ...}
. It's a small change, but it's a nicer and easier to work with structure that reduces connascence.
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 wrote it like this at first ({author_name: ..., author_email: ...}
), and it looked more confusing to me, but I can try reverting it again and seeing how it looks now with the final version.
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 refactored this bit, and I think it looks easier to read now :)
@@ -4,18 +4,58 @@ | |||
require 'datadog/ci/ext/environment' | |||
|
|||
RSpec.describe Datadog::CI::Ext::Environment do | |||
fixture_dir = "#{File.dirname(__FILE__)}/fixtures/" |
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: Suggest turning this into a constant
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.
It's what I've done at first, but Rubocop is mad at it ("Don't declare constants in a dynamic block"), which is technically correct.
I think I can likely tune this specific cop to not check spec files, it might make sense.
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 was going to disable the relevant Rubocop cops for specs, but they are actually quite good at catching bad constant patterns in tests:
spec/datadog/ci/ext/environment_spec.rb:7:3: W: Lint/ConstantDefinitionInBlock: Do not define constants this way within a block.
FIXTURE_DIR = "#{File.dirname(__FILE__)}/fixtures/"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/datadog/ci/ext/environment_spec.rb:7:3: C: RSpec/LeakyConstantDeclaration: Stub constant instead of declaring explicitly.
FIXTURE_DIR = "#{File.dirname(__FILE__)}/fixtures/"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I've change it to a constant, and ignored Rubocop for this case.
I was thinking if there was a way to improve the affected cops upstreams, but I think that someone could easily declare the same FIXTURE_DIR
I did here while trying to actually use it in a test 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.
Yeah, I guess it's a resonable feedback from Rubocop if you don't treat RSpec blocks as anything special.
[core] | ||
repositoryformatversion = 0 | ||
filemode = true | ||
bare = false | ||
logallrefupdates = true | ||
ignorecase = true | ||
precomposeunicode = true |
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.
There's a "yo dawg, I heard you like git..." joke hiding in here somewhere.
b684a53
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.
Left a few more notes :)
lib/datadog/ci/ext/environment.rb
Outdated
def exec_git_command(cmd) | ||
out, status = Open3.capture2e(cmd) | ||
|
||
raise out unless status.success? |
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: Maybe raise "Failed to run git command '#{cmd'}: #{out}"
Datadog::Ext::Git::TAG_COMMIT_AUTHOR_NAME => commit_users[:author_name], | ||
Datadog::Ext::Git::TAG_COMMIT_AUTHOR_EMAIL => commit_users[:author_email], | ||
Datadog::Ext::Git::TAG_COMMIT_AUTHOR_DATE => commit_users[:author_date], | ||
Datadog::Ext::Git::TAG_COMMIT_COMMITTER_NAME => commit_users[:committer_name], | ||
Datadog::Ext::Git::TAG_COMMIT_COMMITTER_EMAIL => commit_users[:committer_email], | ||
Datadog::Ext::Git::TAG_COMMIT_COMMITTER_DATE => commit_users[:committer_date] |
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: Consider using .fetch(...)
since we expect all of these to be available.
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'll follow up on this one on the refactor task that is coming next.
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.
👍
6d1616d
PR reads local metadata from the working directory's git repository, if present, and uses that data to augment any the already present CI provide environment variable information.
CI provider data has priority, so git information can be seen as a fallback.