Skip to content

[feat] - Refactor KumaSDConfigs#7241

Closed
Dharma-09 wants to merge 3 commits intoprometheus-operator:mainfrom
Dharma-09:feat/kumasdconfigs
Closed

[feat] - Refactor KumaSDConfigs#7241
Dharma-09 wants to merge 3 commits intoprometheus-operator:mainfrom
Dharma-09:feat/kumasdconfigs

Conversation

@Dharma-09
Copy link
Contributor

Description

This pull request refactors the validation logic for the KumaSDConfig struct in the Prometheus Operator. The changes include improving the handling of configuration fields such as server, ClientID, TLSConfig , ProxyURL,AllowRedirect.

Fixes

#7203

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

Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.

NONE

@Dharma-09 Dharma-09 requested a review from a team as a code owner December 25, 2024 19:54
@heliapb
Copy link
Member

heliapb commented Dec 27, 2024

Hi @Dharma-09 , you need to run make --always-make format generate in order to update the bundle/docs, and so on

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.

A few comments regarding the e2e tests and version checks.

@mviswanathsai mviswanathsai mentioned this pull request Jan 6, 2025
5 tasks
mviswanathsai
mviswanathsai previously approved these changes Jan 29, 2025
@mviswanathsai
Copy link
Contributor

(nit) could you also add a few tests for the v1.Duration fields?
LGTM otherwise.

CC: @slashpai

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 e2e tests lets add both positive and negative case for each field.

},
},
expectedError: false,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a case for invalid url also?

@Dharma-09
Copy link
Contributor Author

Dharma-09 commented Mar 2, 2025

Hi @mviswanathsai I rebased my branch for PR #7241, but it pulled in some unintended changes. How can I fix this and keep only my intended updates? Also, is there a Slack channel where I can ask for help on issues like this?
Thank you.

@slashpai
Copy link
Contributor

slashpai commented Mar 2, 2025

@Dharma-09 Rebase can be hard sometimes if branch is a bit behind upstream as more changes would have merged.

What I follow to rebase usually is git pull upstream main --rebase and follow with fixing if any conflicts. By default if nothing specified git pull will be using merge strategy to sync your branch which works for some cases. But rebasing is always a better option to have a cleaner git history.

For a easier solution, I would suggest you to create a new branch from current upstream main and cherry-pick your commits from this branch on top of it.

If you are unsure about any git commands its always better to create a backup of current branch, so incase if something is messed up its easy to fix :)

@Dharma-09 Dharma-09 closed this Mar 3, 2025
@Dharma-09 Dharma-09 deleted the feat/kumasdconfigs branch March 3, 2025 20:05
@Dharma-09 Dharma-09 mentioned this pull request Mar 3, 2025
5 tasks
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.

4 participants