Skip to content

Conversation

@Deezzir
Copy link
Contributor

@Deezzir Deezzir commented Sep 16, 2024

  • Adds a new snap config option - dcgm-exporter-metrics-file.
  • Adds a test for the new configuration.
  • Divides tests into two classes: TestDCGMComponents and TestDCGMConfigs.

The user can set that config with the filename of the metrics file placed in /var/snap/dcgm/common prior. The dcgm-exporter wrapper will then check if the file exists in the directory above and supply it to the service.

The custom metrics file we developed will not be added alongside with the PR. It will be moved to the charm later when the feature is merged.

@Deezzir Deezzir marked this pull request as ready for review September 17, 2024 20:12
@Deezzir Deezzir requested a review from a team as a code owner September 17, 2024 20:12
Copy link
Member

@gabrielcocenza gabrielcocenza left a comment

Choose a reason for hiding this comment

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

It's looking good.

It's a personal preference but one of the highlights to me from pytest is that it's not necessary to create classes like when using unittest. I prefer not having classes, but no strong opinion

Copy link
Contributor

@aieri aieri left a comment

Choose a reason for hiding this comment

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

looks good, changes requested simply because I agree with Gabriel's comments

Copy link

@jneo8 jneo8 left a comment

Choose a reason for hiding this comment

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

One small suggestion

@Deezzir Deezzir requested a review from aieri September 18, 2024 22:53
aieri
aieri previously approved these changes Sep 18, 2024
Copy link
Contributor

@aieri aieri left a comment

Choose a reason for hiding this comment

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

nice!

samuelallan72
samuelallan72 previously approved these changes Sep 19, 2024
aieri
aieri previously approved these changes Sep 19, 2024
jneo8
jneo8 previously approved these changes Sep 19, 2024
Copy link
Member

@gabrielcocenza gabrielcocenza left a comment

Choose a reason for hiding this comment

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

Nice job!
Just some small details to polish

@Deezzir Deezzir dismissed stale reviews from jneo8, aieri, and samuelallan72 via 4c09171 September 19, 2024 14:28
@Deezzir Deezzir merged commit c3b3a91 into canonical:main Sep 19, 2024
5 checks passed
@Deezzir Deezzir deleted the metrics-config branch September 19, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants