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

chore: Refactor OpenStackSDConfig #7244

Merged

Conversation

nutmos
Copy link
Contributor

@nutmos nutmos commented Dec 28, 2024

Description

Refactor OpenStackSDConfig

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

  • CHANGE (fix or feature that would cause existing functionality to not work as expected)
  • FEATURE (non-breaking change which adds functionality)
  • BUGFIX (non-breaking change which fixes an issue)
  • ENHANCEMENT (non-breaking change which improves existing functionality)
  • NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Verification

Please check the Prometheus-Operator testing guidelines for recommendations about automated tests.

Changelog entry

1. Add the validation in the type.
2. Add the test cases for type verification.

Fixes #7209

@mviswanathsai
Copy link
Contributor

Hey! thanks for the contribution.
On top of this, you would also need to add e2e test cases and version checks wherever applicable. You can ping me if you need any help with that!

@nutmos nutmos marked this pull request as ready for review December 29, 2024 10:29
@nutmos nutmos requested a review from a team as a code owner December 29, 2024 10:29
@nutmos
Copy link
Contributor Author

nutmos commented Dec 29, 2024

@mviswanathsai Hi, I completed all of the requirements already. Please review.

@nutmos
Copy link
Contributor Author

nutmos commented Jan 22, 2025

@mviswanathsai Hi, do you have any chance to take a look at my PR?

Copy link
Contributor

@slashpai slashpai left a comment

Choose a reason for hiding this comment

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

For string fields we need to add the minlength validation

// +kubebuilder:validation:MinLength:=1

@nutmos nutmos force-pushed the chore/refactor-openstack-sd-config branch from 67501de to 3ad9eda Compare January 29, 2025 13:17
@nutmos nutmos requested a review from slashpai January 29, 2025 14:41
@nutmos
Copy link
Contributor Author

nutmos commented Jan 30, 2025

@mviswanathsai @slashpai Hi, I fixed already. Please review. Thank you!

@slashpai
Copy link
Contributor

Can you add testcases for all the fields updated in API in e2e test file?

@nutmos nutmos force-pushed the chore/refactor-openstack-sd-config branch from df8b8f7 to 59a82e9 Compare January 30, 2025 06:41
@nutmos
Copy link
Contributor Author

nutmos commented Jan 30, 2025

@slashpai I updated the pattern as suggested already along with the new test cases to cover field updates.
Also, I would like to highlight that there are other HTTP-related fields use this pattern, which I will help updating them later as well.

@nutmos nutmos requested a review from slashpai January 30, 2025 10:43
Copy link
Contributor

@mviswanathsai mviswanathsai left a comment

Choose a reason for hiding this comment

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

Great job with the extensive testcases, just one comment.
CC: @slashpai

@nutmos nutmos requested a review from mviswanathsai January 30, 2025 13:02
@nutmos
Copy link
Contributor Author

nutmos commented Jan 30, 2025

@mviswanathsai I added 2 more cases to cover the missing required fields.

@nutmos nutmos force-pushed the chore/refactor-openstack-sd-config branch from 1da29f8 to 25528db Compare February 1, 2025 10:07
@nutmos
Copy link
Contributor Author

nutmos commented Feb 1, 2025

@mviswanathsai Update the code with rebase from the main branch. Please review.

@nutmos nutmos force-pushed the chore/refactor-openstack-sd-config branch from 39a76d8 to 0f6c0fa Compare February 2, 2025 04:22
@mviswanathsai
Copy link
Contributor

LGTM!
CC: @slashpai

@nutmos
Copy link
Contributor Author

nutmos commented Feb 4, 2025

@slashpai any action needed from my side?

Copy link
Contributor

@slashpai slashpai left a comment

Choose a reason for hiding this comment

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

Thank you, lgtm

@slashpai slashpai merged commit 6f3c19a into prometheus-operator:main Feb 4, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor OpenStackSDConfig
3 participants