-
Notifications
You must be signed in to change notification settings - Fork 722
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
Draft of ES declarative config #3775
Conversation
@@ -171,6 +171,7 @@ go-run: | |||
-tags "$(GO_TAGS)" \ | |||
./cmd/main.go manager \ | |||
--development \ | |||
--enable-leader-election=false \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just to enable quicker iteration when running locally and could be moved to a separate PR
I added a few comments about the general approach in the issue: #3598 (comment) |
} | ||
|
||
// TODO checking for a 200 might be wrong, really any status code not 200 might mean we need to update. but a 200 indicates we can and should compare the bodies | ||
// TODO should bodies always be required to be specified? I think probably? I don't know of any ES API that you can send an empty PUT. maybe we require at a minimum "{}"? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll go ahead and close this out while we investigate an alternative approach (a single fire and forget job) that I'll detail in the issue |
Fix #3598
Opening up a draft to get feedback on the approach, still needs a lot of shoring up to do. But it "works".
An overview
Example manual test
Open questions
does the association interface make sense to use here? it has a meaning in other resources because the operator is configuring a resource (e.g. kibana) to create an association. here, the operator is making the calls itself, so it has a slightly different meaning. I cargo culted it in because I was used to it, but thinking about it more I'm not sure it's necessary
ES sometimes supports additional arguments in PUTs that it does not support for GETs (e.g.
verify=false
for snapshot repo creation), but we provide no mechanism for that here. It's not clear to me how important that might be though.does checking specifically for 404s/200s make sense? I'm not sure if there's other status codes ES use that make sense for us to proceed on
now is a good time for naming/structure feedback too