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

[cleanup] move constants to feature-specific files #1444

Merged
merged 3 commits into from
Oct 8, 2024

Conversation

celenechang
Copy link
Contributor

What does this PR do?

Move feature-specific constants from api/common to be near where they are used

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Minimum Agent Versions

Are there minimum versions of the Datadog Agent and/or Cluster Agent required?

  • Agent: vX.Y.Z
  • Cluster Agent: vX.Y.Z

Describe your test plan

Ensure the change is noop, i.e. the binary builds and runs without issue

Checklist

  • PR has at least one valid label: bug, enhancement, refactoring, documentation, tooling, and/or dependencies
  • PR has a milestone or the qa/skip-qa label

@celenechang celenechang added this to the v1.10.0 milestone Oct 1, 2024
@celenechang celenechang requested review from a team as code owners October 1, 2024 11:26
InstallInfoVolumeSubPath = "install_info"
InstallInfoVolumePath = "/etc/datadog-agent/install_info"
InstallInfoVolumeReadOnly = true

DogstatsdHostPortName = "dogstatsdport"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should some of the dogstatsd constants be moved to the feature's const file? DogstatsdHostPortName is only used in dsd's feature.go and test file, but some others are used elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, i intentionally punted on the ones that require some thought, for now..

@@ -24,7 +24,7 @@ const (
defaultLogContainerLogsPath string = "/var/lib/docker/containers"
defaultLogPodLogsPath string = "/var/log/pods"
defaultLogContainerSymlinksPath string = "/var/log/containers"
defaultLogTempStoragePath string = "/var/lib/datadog-agent/logs"
DefaultLogTempStoragePath string = "/var/lib/datadog-agent/logs"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this go in api/datadoghq/common/const.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think you're right that it would be a better place for it


apmHostPortName = "traceport"
apmSocketVolumeName = "apmsocket"
apmSocketVolumeLocalPath = "/var/run/datadog"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be combined with dsd like we do with the hostpath DogstatsdAPMSocketHostPath? Or would it be better to separate DogstatsdAPMSocketHostPath into dsd and apm specific paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm not sure, but i guess we can decide and stick with one approach. it certainly seems there is some deduplication we can do for this particular constant

@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2024

Codecov Report

Attention: Patch coverage is 85.10638% with 7 lines in your changes missing coverage. Please review.

Project coverage is 49.01%. Comparing base (1d9b69b) to head (d89e2c2).

Files with missing lines Patch % Lines
...al/controller/datadogagent/feature/otlp/feature.go 66.66% 2 Missing ⚠️
...nal/controller/datadogagent/feature/apm/feature.go 66.66% 1 Missing ⚠️
...al/controller/datadogagent/feature/cspm/feature.go 75.00% 1 Missing ⚠️
...nal/controller/datadogagent/feature/cws/feature.go 85.71% 1 Missing ⚠️
...atadogagent/feature/kubernetesstatecore/feature.go 66.66% 1 Missing ⚠️
...tadogagent/feature/orchestratorexplorer/feature.go 66.66% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1444   +/-   ##
=======================================
  Coverage   49.01%   49.01%           
=======================================
  Files         223      223           
  Lines       19508    19508           
=======================================
  Hits         9562     9562           
  Misses       9456     9456           
  Partials      490      490           
Flag Coverage Δ
unittests 49.01% <85.10%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
api/datadoghq/v2alpha1/datadogagent_default.go 89.74% <100.00%> (ø)
internal/controller/datadogagent/common/volumes.go 100.00% <ø> (ø)
...atadogagent/feature/admissioncontroller/feature.go 79.52% <100.00%> (ø)
...ernal/controller/datadogagent/feature/apm/const.go 100.00% <ø> (ø)
...rnal/controller/datadogagent/feature/cspm/const.go 50.00% <ø> (ø)
...ller/datadogagent/feature/eventcollection/const.go 100.00% <ø> (ø)
...er/datadogagent/feature/eventcollection/feature.go 62.31% <100.00%> (ø)
...er/datadogagent/feature/externalmetrics/feature.go 54.16% <100.00%> (ø)
...controller/datadogagent/feature/helmcheck/const.go 100.00% <ø> (ø)
...ntroller/datadogagent/feature/helmcheck/feature.go 78.65% <100.00%> (ø)
... and 10 more

Continue to review full report in Codecov by Sentry.

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

@celenechang celenechang merged commit 172caff into main Oct 8, 2024
19 checks passed
@celenechang celenechang deleted the celene/feature_constants branch October 8, 2024 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants