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

Update createOrReplace to work more like BaseOperation #2289

Merged
merged 1 commit into from Jun 19, 2020

Conversation

bbeaudreault
Copy link
Contributor

This fixes #2285

Rather than try to actually handle exception propagation for create-then-replace, I decided to mirror the BaseOperation implementation which does a get then branch accordingly. I added a couple tests.

@centos-ci
Copy link

Can one of the admins verify this patch?

@rohanKanojia
Copy link
Member

Ok to test

@manusa
Copy link
Member

manusa commented Jun 16, 2020

I see the approach of get and then do create / update accordingly if the object exists or not.

I only see one flaw to this approach which is if the object is created (by another thread, process, service, etc.) in the time between get returns null and the current thread/process tries to create the object.

Is it possible to use the try-to-create-or-edit approach by detecting if the POST request failed due to the resource already being there? (I don't know if k8s returns a 409 status or something that will allow us to detect such a case)

Something like:

  1. Try to create
    2.A.1. Create works
    2.A.2. Method returns
    2.B.1 Create fails due to conflict
    2.B.2 Try with edit
    2.B......
    2.C.1 Create fails due to some other error
    2.C.2 Throw exception

@bbeaudreault
Copy link
Contributor Author

I agree, that was my original thought as well, but decided to mirror BaseOperation for uniformity. But I will update to use that approach. We should probably update BaseOperation as well, but I don't have time to do that in this PR right now.

@manusa
Copy link
Member

manusa commented Jun 16, 2020

I agree, that was my original thought as well...

Thanks, I created an issue (#2292) to keep track of the change.

@bbeaudreault
Copy link
Contributor Author

This has been updated as requested

@sonarcloud
Copy link

sonarcloud bot commented Jun 17, 2020

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 2 Code Smells

57.1% 57.1% Coverage
0.0% 0.0% Duplication

@bbeaudreault
Copy link
Contributor Author

@rohanKanojia this is ready. can you re-run the 3.11 test?

the remaining 2 code smell seem more trouble than they are worth

Copy link
Member

@rohanKanojia rohanKanojia 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, Thanks

Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

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

LGTM, thank you very much!

@rohanKanojia
Copy link
Member

[merge]

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.

Raw Custom Resources API createOrReplace does not handle create failures gracefully
5 participants