-
Notifications
You must be signed in to change notification settings - Fork 115
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
Allow skipAwait to skip only readiness or deletion #3147
Conversation
This change is part of the following stack: Change managed by git-spice. |
1d3e62b
to
2ca7228
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## blampe/2996-await-pr #3147 +/- ##
========================================================
+ Coverage 38.14% 38.18% +0.03%
========================================================
Files 77 77
Lines 9361 9367 +6
========================================================
+ Hits 3571 3577 +6
Misses 5444 5444
Partials 346 346 ☔ View full report in Codecov by Sentry. |
8c4de52
to
42e88ad
Compare
2ca7228
to
eb3b1f7
Compare
42e88ad
to
6377940
Compare
eb3b1f7
to
f185dee
Compare
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'm pretty reluctant to skip await on deletion unless the user has explicitly opted in with skipAwait: delete
. Because skipping await on deletion can lead to a race condition in the 'replacement' use case.
I think the safe move is to interpret the value of true
as ready
, not as ready,delete
. Since that is a breaking change, another possibility would be to interpret true
differently depending on the kind, to retain existing behavior. I'm still surprised that skipAwait ever applied to deletes.
In the future, maybe we should consider orphaning CRDs rather than deleting them by default, as is the behavior of Helm. This would solve one of the motivations for skipAwait applying to deletes.
provider/pkg/metadata/annotations.go
Outdated
if IsAnnotationTrue(inputs, AnnotationSkipAwait) { | ||
return true | ||
} | ||
return strings.EqualFold(GetAnnotationValue(inputs, AnnotationSkipAwait), "ready") |
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 would advocate for parsing the value as a comma-separated string, and then checking for the enum values of true, ready, delete. This would allow for a user to do ready,delete
and for us to contemplate treating true
as ready
.
6377940
to
e0cd689
Compare
f185dee
to
14de8bf
Compare
e0cd689
to
eaa802e
Compare
14de8bf
to
f3a721d
Compare
eaa802e
to
9a5a997
Compare
f3a721d
to
b912dd2
Compare
On pause #2551 (comment) |
Currently, the
pulumi.com/skipAwait: true
annotation tells the provider toskip await logic whenever a resource is touched or deleted.
However, there are situations where you might want to skip await logic for only
deletes, or only for readiness, but not both. For example a resource might take
a long time to tear down, during which time other resources could also be
getting cleaned up.
This PR expands the acceptable values for the
skipAwait
annotation:"pulumi.com/skipAwait": "true"
continues to skip await logic in all situations."pulumi.com/skipAwait": "ready"
will skip await logic when creating orupdating a resource, when we would normally wait for it to become ready. We
will still wait for deletion.
"pulumi.com/skipAwait": "delete"
will skip await logic when deleting theresource, but we will continue to wait for it become ready when creating or
updating it.
Fixes #2551.
Refs #2824.
Refs #1418.