-
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-visibility] Update CI Visibility spec #2874
[ci-visibility] Update CI Visibility spec #2874
Conversation
8c2b36f
to
042766d
Compare
lib/datadog/ci/ext/environment.rb
Outdated
.sort_by { |key| key.length } | ||
|
||
if extra_tags.length > 0 | ||
tags[TAG_NODE_LABELS] = JSON.generate(extra_tags) |
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.
JSON.generate(extra_tags)
and extra_tags.to_json
are analogous. Since it looks like we are using to_json
in other parts of this file, should we try being consistent and use to_json
here as well 😄 ?
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 tried that first, but it seems .to_json
does not work for lists. .to_s
seems to work but it adds some extra spaces I don't want: maybe there's a workaround?
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.
Interesting
I just did a simple example in irb
require 'json'
=> true
irb(main):002:0> test = ["mytag:my-value", "myothertag:my-other-value"]
=> ["mytag:my-value", "myothertag:my-other-value"]
irb(main):003:0> test.to_json
=> "[\"mytag:my-value\",\"myothertag:my-other-value\"]"
That is what you want, right?
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.
ok I messed it up somehow 😆 . I'll change it, thanks
lib/datadog/ci/ext/environment.rb
Outdated
@@ -308,6 +325,11 @@ def extract_jenkins(env) | |||
name = name.gsub("/#{normalize_ref(branch)}", '') if branch | |||
name = name.split('/').reject { |v| v.nil? || v.include?('=') }.join('/') | |||
end | |||
|
|||
if !env['NODE_LABELS'].nil? | |||
node_labels = JSON.generate(env['NODE_LABELS'].split(' ')) |
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 #2874 +/- ##
=======================================
Coverage 98.09% 98.09%
=======================================
Files 1269 1264 -5
Lines 69969 69941 -28
Branches 3161 3162 +1
=======================================
- Hits 68633 68607 -26
+ Misses 1336 1334 -2 see 25 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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
What does this PR do?
ci.node.labels
andci.node.name
from buildkite, gitlab and jenkins.Motivation
Keep up with CI Visibility spec changes
How to test the change?
Unit tests via fixtures.