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

[CIAPP-5371] CI visibility: validate git tags #3100

Merged
merged 2 commits into from
Sep 4, 2023

Conversation

anmarchenko
Copy link
Member

@anmarchenko anmarchenko commented Sep 1, 2023

What does this PR do?

In cases when it is not possible to automatically extract git environment information for CI visibility we allow users to supply this info via environment variables, see docs here.

DD_GIT_REPOSITORY_URL and DD_GIT_COMMIT_SHA are required but currently not validated. When these values are not provided or SHA is not valid CI-APP backend silently drops these traces. End users get no feedback in these cases.

We add the following validations to these tags:

  • DD_GIT_REPOSITORY_URL must be present
  • DD_GIT_COMMIT_SHA must be present, must be 40 characters long must be a valid hex number

In case these values are incorrect we continue to send traces but we log an error message explaining the problem.

Motivation:

Improve end user experience and prevent confusion when test spans are not shown in UI without any reason.

How to test the change?
Covered by unit tests.

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

@github-actions github-actions bot added the ci-app CI product for test suite instrumentation label Sep 1, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #3100 (534ddb0) into master (90cf733) will decrease coverage by 0.01%.
Report is 17 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3100      +/-   ##
==========================================
- Coverage   98.15%   98.15%   -0.01%     
==========================================
  Files        1323     1323              
  Lines       75048    75068      +20     
  Branches     3422     3423       +1     
==========================================
+ Hits        73665    73684      +19     
- Misses       1383     1384       +1     
Files Changed Coverage Δ
lib/datadog/ci/ext/environment.rb 98.93% <100.00%> (+0.01%) ⬆️
spec/datadog/ci/ext/environment_spec.rb 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions github-actions bot added the core Involves Datadog core libraries label Sep 1, 2023
@anmarchenko anmarchenko marked this pull request as ready for review September 1, 2023 10:47
@anmarchenko anmarchenko requested a review from a team September 1, 2023 10:47
is_expected.to eq(
{
'ci.workspace_path' => "#{Dir.pwd}/spec/datadog/ci/ext/fixtures/git",
'git.branch' => env['DD_GIT_BRANCH'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change the assertion with the actual value instead of an indirection through environment variable?

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry, I missed your comment :(

I think in this case though it would make it only more magical to put an actual value here as these env vars are defined in the same context

@anmarchenko anmarchenko merged commit f2c8241 into master Sep 4, 2023
@anmarchenko anmarchenko deleted the anmarchenko/ci_vis_validate_git_tags branch September 4, 2023 12:35
@github-actions github-actions bot added this to the 1.15.0 milestone Sep 4, 2023
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 core Involves Datadog core libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants