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.

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

Merged
merged 4 commits into from
Jun 22, 2021
Merged

CI-App:Read metadata from local git repository #1561

merged 4 commits into from
Jun 22, 2021

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented Jun 18, 2021

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.

@marcotc marcotc requested a review from a team June 18, 2021 23:55
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',
Copy link
Member Author

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?

Copy link
Contributor

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?

@marcotc marcotc requested a review from a team June 18, 2021 23:56
@marcotc marcotc self-assigned this Jun 18, 2021
@marcotc marcotc added the ci-app CI product for test suite instrumentation label Jun 18, 2021
@@ -312,6 +317,107 @@ def extract_bitrise(env)
}
end

def git_commit_users
Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

🙏

@codecov-commenter
Copy link

codecov-commenter commented Jun 18, 2021

Codecov Report

Merging #1561 (f343613) into master (bd7eee4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1561   +/-   ##
=======================================
  Coverage   98.24%   98.24%           
=======================================
  Files         885      885           
  Lines       42592    42674   +82     
=======================================
+ Hits        41843    41926   +83     
+ Misses        749      748    -1     
Impacted Files Coverage Δ
lib/datadog/ci/ext/environment.rb 99.36% <100.00%> (+1.16%) ⬆️
lib/ddtrace/ext/git.rb 100.00% <100.00%> (ø)
spec/datadog/ci/ext/environment_spec.rb 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd7eee4...f343613. Read the comment docs.

juan-fernandez
juan-fernandez previously approved these changes Jun 21, 2021
Copy link
Contributor

@juan-fernandez juan-fernandez left a comment

Choose a reason for hiding this comment

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

looking good!

ivoanjo
ivoanjo previously approved these changes Jun 21, 2021
Copy link
Member

@ivoanjo ivoanjo left a 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 👍

lib/datadog/ci/ext/environment.rb Outdated Show resolved Hide resolved
@@ -312,6 +317,107 @@ def extract_bitrise(env)
}
end

def git_commit_users
Copy link
Member

Choose a reason for hiding this comment

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

🙏

Comment on lines 323 to 324
# 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.
Copy link
Member

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

Copy link
Member Author

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 Show resolved Hide resolved
Comment on lines 383 to 389
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)
Copy link
Member

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)

Copy link
Member Author

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.

Comment on lines 412 to 417
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)
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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/"
Copy link
Member

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

Copy link
Member Author

@marcotc marcotc Jun 21, 2021

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.

Copy link
Member Author

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.

Copy link
Member

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.

spec/datadog/ci/ext/environment_spec.rb Outdated Show resolved Hide resolved
spec/datadog/ci/ext/environment_spec.rb Outdated Show resolved Hide resolved
Comment on lines +1 to +7
[core]
repositoryformatversion = 0
filemode = true
bare = false
logallrefupdates = true
ignorecase = true
precomposeunicode = true
Copy link
Member

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.

ericmustin
ericmustin previously approved these changes Jun 21, 2021
@marcotc marcotc dismissed stale reviews from ericmustin, ivoanjo, and juan-fernandez via b684a53 June 21, 2021 19:30
@marcotc
Copy link
Member Author

marcotc commented Jun 21, 2021

I updated our integration test app (an RSpec app), and capture this action shot with this new feature:
Screen Shot 2021-06-21 at 2 28 25 PM

ericmustin
ericmustin previously approved these changes Jun 21, 2021
ivoanjo
ivoanjo previously approved these changes Jun 22, 2021
Copy link
Member

@ivoanjo ivoanjo left a 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 :)

integration/apps/rspec/bin/setup Show resolved Hide resolved
def exec_git_command(cmd)
out, status = Open3.capture2e(cmd)

raise out unless status.success?
Copy link
Member

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}"

lib/datadog/ci/ext/environment.rb Outdated Show resolved Hide resolved
Comment on lines +410 to +415
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]
Copy link
Member

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.

Copy link
Member Author

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.

juan-fernandez
juan-fernandez previously approved these changes Jun 22, 2021
Copy link
Contributor

@juan-fernandez juan-fernandez left a comment

Choose a reason for hiding this comment

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

👍

@marcotc marcotc dismissed stale reviews from juan-fernandez, ivoanjo, and ericmustin via 6d1616d June 22, 2021 20:49
@marcotc marcotc merged commit 70032cf into master Jun 22, 2021
@marcotc marcotc deleted the local-ci-git branch June 22, 2021 21:02
@github-actions github-actions bot added this to the 0.51.0 milestone Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-app CI product for test suite instrumentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants