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

Fix symbol configuration for env and service #2864

Merged
merged 4 commits into from
May 23, 2023

Conversation

TonyCTHsu
Copy link
Contributor

What does this PR do?

  1. Make env and service configuration backward compatible for symbol
  2. Add missing spec for SafeDup

@TonyCTHsu TonyCTHsu requested a review from a team May 19, 2023 12:16
@TonyCTHsu TonyCTHsu self-assigned this May 19, 2023
@github-actions github-actions bot added the core Involves Datadog core libraries label May 19, 2023
@TonyCTHsu TonyCTHsu linked an issue May 19, 2023 that may be closed by this pull request
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.

Looking good! Just a few suggestions :)

lib/datadog/core/configuration/settings.rb Show resolved Hide resolved
spec/datadog/core/utils/safe_dup_spec.rb Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #2864 (8bb2867) into master (51aeeda) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2864   +/-   ##
=======================================
  Coverage   98.08%   98.09%           
=======================================
  Files        1269     1270    +1     
  Lines       69963    70017   +54     
  Branches     3160     3163    +3     
=======================================
+ Hits        68624    68683   +59     
+ Misses       1339     1334    -5     
Impacted Files Coverage Δ
lib/datadog/core/configuration/settings.rb 99.29% <100.00%> (+0.01%) ⬆️
lib/datadog/core/telemetry/collector.rb 100.00% <100.00%> (ø)
lib/datadog/core/telemetry/v1/dependency.rb 100.00% <100.00%> (ø)
spec/datadog/core/configuration/settings_spec.rb 100.00% <100.00%> (ø)
spec/datadog/core/telemetry/v1/dependency_spec.rb 100.00% <100.00%> (ø)
spec/datadog/core/utils/safe_dup_spec.rb 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

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

it do
input = 'a_string'
context 'when given a string' do
it 'returns a duplicate' do
Copy link
Member

Choose a reason for hiding this comment

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

returns a non frozen dupliacte

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 extra suggestions, but overall LGTM 👍, thanks for this!

Comment on lines 11 to 12
expect(result).to eq(input)
expect(result).to equal(input)
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 can be simplified a bit, because we only need to check #equal?, as that's the one that tests the object references and should never be overridden.

Rspec has a nice alias for equal that I like to use in these situations:

Suggested change
expect(result).to eq(input)
expect(result).to equal(input)
expect(result).to be(input)

result = described_class.frozen_or_dup(input)

expect(result).to eq(input)
expect(result).not_to equal(input)
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Suggestion to match my other suggestion above

Suggested change
expect(result).not_to equal(input)
expect(result).not_to be(input)

Comment on lines 37 to 38
expect(result).to eq(input)
expect(result).to equal(input)
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Suggestion to match my other suggestion above

Suggested change
expect(result).to eq(input)
expect(result).to equal(input)
expect(result).to be(input)

expect(result).to eq(input)
expect(result).not_to equal(input)
expect(result).not_to be_frozen
end
Copy link
Member

Choose a reason for hiding this comment

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

Minor: May be worth adding here a test that validates that the input string is not frozen in the process.

(And the same for #frozen_dup below)

result = described_class.frozen_dup(input)

expect(result).to eq(input)
expect(result).not_to equal(input)
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Suggestion to match my other suggestion above

Suggested change
expect(result).not_to equal(input)
expect(result).not_to be(input)

@TonyCTHsu TonyCTHsu merged commit c38d420 into master May 23, 2023
200 of 201 checks passed
@TonyCTHsu TonyCTHsu deleted the tonycthsu/fix-symbol-configuration branch May 23, 2023 12:27
@github-actions github-actions bot added this to the 1.12.0 milestone May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

env & service configured as symbols break log correlation in 1.11.0
4 participants