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

marcotc · 2021-06-18T23:55:12Z

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 · 2021-06-18T23:56:04Z

spec/datadog/ci/ext/environment_spec.rb

+      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',


@juan-fernandez Is this date format what we expect for the CI app?

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 · 2021-06-18T23:56:49Z

lib/datadog/ci/ext/environment.rb

@@ -312,6 +317,107 @@ def extract_bitrise(env)
          }
        end

+        def git_commit_users


I'll refactor this whole file into separate "extractors" in a follow up PR.

codecov-commenter · 2021-06-18T23:56:56Z

Codecov Report

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

@@           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

looking good!

ivoanjo

Left a few small notes, but overall it LGTM 👍

lib/datadog/ci/ext/environment.rb

ivoanjo · 2021-06-21T07:54:17Z

lib/datadog/ci/ext/environment.rb

@@ -312,6 +317,107 @@ def extract_bitrise(env)
          }
        end

+        def git_commit_users


ivoanjo · 2021-06-21T07:58:02Z

lib/datadog/ci/ext/environment.rb

+          # 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.


Minor: Consider moving this right next to the date conversion, rather than just having it randomly here

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

ivoanjo · 2021-06-21T08:09:00Z

lib/datadog/ci/ext/environment.rb

+        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)


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)

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.

ivoanjo · 2021-06-21T08:13:38Z

lib/datadog/ci/ext/environment.rb

+            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)


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.

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.

I refactored this bit, and I think it looks easier to read now :)

ivoanjo · 2021-06-21T08:16:15Z

spec/datadog/ci/ext/environment_spec.rb

@@ -4,18 +4,58 @@
 require 'datadog/ci/ext/environment'

 RSpec.describe Datadog::CI::Ext::Environment do
+  fixture_dir = "#{File.dirname(__FILE__)}/fixtures/"


Minor: Suggest turning this into a constant

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.

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.

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

ivoanjo · 2021-06-21T08:28:09Z

spec/datadog/ci/ext/fixtures/git/gitdir_empty/config

+[core]
+	repositoryformatversion = 0
+	filemode = true
+	bare = false
+	logallrefupdates = true
+	ignorecase = true
+	precomposeunicode = true


There's a "yo dawg, I heard you like git..." joke hiding in here somewhere.

marcotc · 2021-06-21T21:29:20Z

I updated our integration test app (an RSpec app), and capture this action shot with this new feature:

ivoanjo

Left a few more notes :)

integration/apps/rspec/bin/setup

ivoanjo · 2021-06-22T07:48:25Z

lib/datadog/ci/ext/environment.rb

+        def exec_git_command(cmd)
+          out, status = Open3.capture2e(cmd)
+
+          raise out unless status.success?


Minor: Maybe raise "Failed to run git command '#{cmd'}: #{out}"

lib/datadog/ci/ext/environment.rb

ivoanjo · 2021-06-22T07:50:14Z

lib/datadog/ci/ext/environment.rb

+              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]


Minor: Consider using .fetch(...) since we expect all of these to be available.

I'll follow up on this one on the refactor task that is coming next.

juan-fernandez

👍

CI-App:Read metadata from local git repository

683db3e

marcotc requested a review from a team June 18, 2021 23:55

marcotc commented Jun 18, 2021

View reviewed changes

marcotc requested a review from a team June 18, 2021 23:56

marcotc self-assigned this Jun 18, 2021

marcotc added the ci-app CI product for test suite instrumentation label Jun 18, 2021

marcotc commented Jun 18, 2021

View reviewed changes

juan-fernandez previously approved these changes Jun 21, 2021

View reviewed changes

ivoanjo previously approved these changes Jun 21, 2021

View reviewed changes

ericmustin previously approved these changes Jun 21, 2021

View reviewed changes

Address PR comments

b684a53

marcotc dismissed stale reviews from ericmustin, ivoanjo, and juan-fernandez via b684a53 June 21, 2021 19:30

Add git information to integration test

f343613

marcotc requested review from ivoanjo and juan-fernandez June 21, 2021 21:29

ericmustin previously approved these changes Jun 21, 2021

View reviewed changes

ivoanjo previously approved these changes Jun 22, 2021

View reviewed changes

juan-fernandez previously approved these changes Jun 22, 2021

View reviewed changes

More feedback

6d1616d

marcotc dismissed stale reviews from juan-fernandez, ivoanjo, and ericmustin via 6d1616d June 22, 2021 20:49

marcotc merged commit 70032cf into master Jun 22, 2021

marcotc deleted the local-ci-git branch June 22, 2021 21:02

github-actions bot added this to the 0.51.0 milestone Jun 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

marcotc commented Jun 18, 2021

marcotc Jun 18, 2021

juan-fernandez Jun 21, 2021

marcotc Jun 18, 2021

ivoanjo Jun 21, 2021

codecov-commenter commented Jun 18, 2021 •

edited

Loading

juan-fernandez left a comment

ivoanjo left a comment

ivoanjo Jun 21, 2021

ivoanjo Jun 21, 2021

marcotc Jun 21, 2021

ivoanjo Jun 21, 2021

marcotc Jun 21, 2021

ivoanjo Jun 21, 2021

marcotc Jun 21, 2021

marcotc Jun 21, 2021

ivoanjo Jun 21, 2021

marcotc Jun 21, 2021 •

edited

Loading

marcotc Jun 21, 2021

ivoanjo Jun 22, 2021

ivoanjo Jun 21, 2021

marcotc commented Jun 21, 2021

ivoanjo left a comment

ivoanjo Jun 22, 2021

ivoanjo Jun 22, 2021

marcotc Jun 22, 2021

juan-fernandez left a comment

		# 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.

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

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

Conversation

marcotc commented Jun 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 18, 2021 • edited Loading

Codecov Report

juan-fernandez left a comment

Choose a reason for hiding this comment

ivoanjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcotc Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcotc commented Jun 21, 2021

ivoanjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juan-fernandez left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 18, 2021 •

edited

Loading

marcotc Jun 21, 2021 •

edited

Loading