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

Convert servicemonitor #1250

Closed
wants to merge 4 commits into from
Closed

Convert servicemonitor #1250

wants to merge 4 commits into from

Conversation

alejandroEsc
Copy link
Contributor

@alejandroEsc alejandroEsc commented Apr 25, 2024

Done, generating code is working.

charts/redpanda/servicemonitor.go Outdated Show resolved Hide resolved
charts/redpanda/servicemonitor.go Outdated Show resolved Hide resolved
charts/redpanda/servicemonitor.go Outdated Show resolved Hide resolved
charts/redpanda/values.go Outdated Show resolved Hide resolved
@alejandroEsc alejandroEsc force-pushed the ae/convert-servicemonitor branch 4 times, most recently from 70e675c to fdead49 Compare April 29, 2024 12:59
@alejandroEsc alejandroEsc changed the title [WIP] Convert servicemonitor Convert servicemonitor Apr 30, 2024
@chrisseto
Copy link
Contributor

@alejandroEsc could you clean up the commit history? Looks like you've pulled in a few things from main that are muddying the diff.

@alejandroEsc
Copy link
Contributor Author

@alejandroEsc could you clean up the commit history? Looks like you've pulled in a few things from main that are muddying the diff.

maybe i did, but i had not even rebased when those showed up, not sure what happened

charts/redpanda/values.go Show resolved Hide resolved
charts/redpanda/values.go Outdated Show resolved Hide resolved
charts/redpanda/servicemonitor.go Show resolved Hide resolved
Copy link
Contributor

@chrisseto chrisseto left a comment

Choose a reason for hiding this comment

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

Ah, there's one missing part here 😓 Sorry there's not great documentation here.

You'll need to modify the original servicemonitor.yaml to instead call out to the go function. There's not currently support for marking a function as an "entrypoint" or anything like that. service.nodeport.yaml has an example of what that looks like.

Could we also add some more tests for this by creating some novalues.yaml files?

cmd/genschema/main.go Outdated Show resolved Hide resolved
charts/redpanda/servicemonitor.go Show resolved Hide resolved
charts/redpanda/servicemonitor.go Show resolved Hide resolved
Copy link
Contributor

@chrisseto chrisseto left a comment

Choose a reason for hiding this comment

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

LGTM!

Looks like you'll need to run go mod tidy in the testdata directory and you've got an interesting test failure that I'm not immediately recognizing.

@chrisseto
Copy link
Contributor

closing in favor of #1313

@chrisseto chrisseto closed this May 23, 2024
@RafalKorepta RafalKorepta deleted the ae/convert-servicemonitor branch August 6, 2024 22:17
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.

2 participants