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

User-defined await logic #3134

Merged
merged 5 commits into from
Aug 21, 2024
Merged

User-defined await logic #3134

merged 5 commits into from
Aug 21, 2024

Conversation

blampe
Copy link
Contributor

@blampe blampe commented Jul 26, 2024

This adds implementations for user-defined await logic.

Two styles are supported, both equivalent to kubectl:

  • condition=Type[=Value]
  • jsonpath={expr}[=Value]

The user can specify these via annotations, e.g.:

  • pulumi.com/waitFor: condition=Foo
  • pulumi.com/waitFor: jsonpath={.webhooks[0].clientConfig.caBundle}

kubectl --wait for=jsonpath= supports a more relaxed syntax than kubectl get -o jsonpath= which I found to be somewhat buggy/surprising in how it parsed
expressions, so for now we only support the stricter get syntax.

Some code related to evaluating custom conditions is vendored from kubectl to
stay as close as possible to upstream behavior.

We use the same k8s.io/client-go/util/jsonpath library used by kubectl for
evaluating JSONPath expressions.

An E2E test is included which shows how this feature allows cert-manager to be
successfully deployed after applying two waitFor annotations. (Something we
would eventually want baked in to pulumi-kubernetes-cert-manager.)

Fixes #1260.
Refs #2824.

@blampe
Copy link
Contributor Author

blampe commented Jul 26, 2024

This change is part of the following stack:

Change managed by git-spice.

@blampe blampe force-pushed the blampe/2996-await-delete branch from 71719cb to 7affd4c Compare July 26, 2024 20:18
@blampe blampe force-pushed the blampe/2996-await-delete branch from 7affd4c to 5c06e56 Compare July 26, 2024 20:28
@blampe blampe force-pushed the blampe/2996-await-custom branch from 06828f1 to 7268577 Compare July 26, 2024 20:30
@blampe blampe force-pushed the blampe/2996-await-delete branch from 5c06e56 to 49579dd Compare July 26, 2024 20:37
@blampe blampe force-pushed the blampe/2996-await-custom branch from 7268577 to b065c74 Compare July 26, 2024 20:38
@blampe blampe added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Jul 29, 2024
@blampe blampe force-pushed the blampe/2996-await-delete branch from 49579dd to 1d398f2 Compare July 29, 2024 21:58
@blampe blampe force-pushed the blampe/2996-await-custom branch 2 times, most recently from 5072687 to 847138c Compare July 29, 2024 22:11
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@blampe blampe force-pushed the blampe/2996-await-delete branch from 5322331 to b1d2cfb Compare July 31, 2024 18:58
@blampe blampe mentioned this pull request Jul 31, 2024
1 task
@blampe blampe changed the base branch from blampe/2996-await-delete to blampe/2996-await-pr July 31, 2024 19:33
@blampe blampe force-pushed the blampe/2996-await-custom branch from 847138c to 57fe639 Compare July 31, 2024 19:33
@blampe blampe requested a review from rquitales July 31, 2024 19:34
@blampe blampe marked this pull request as ready for review July 31, 2024 19:34
@blampe blampe force-pushed the blampe/2996-await-pr branch from 8c4de52 to 42e88ad Compare August 1, 2024 19:53
@blampe blampe force-pushed the blampe/2996-await-custom branch from 57fe639 to 253a623 Compare August 1, 2024 19:53
@blampe blampe removed the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Aug 1, 2024
Copy link

codecov bot commented Aug 1, 2024

Codecov Report

Attention: Patch coverage is 85.10638% with 28 lines in your changes missing coverage. Please review.

Project coverage is 39.08%. Comparing base (a8234db) to head (41bd8fe).
Report is 1 commits behind head on master.

Files Patch % Lines
provider/pkg/await/condition/all.go 64.28% 6 Missing and 4 partials ⚠️
provider/pkg/metadata/overrides.go 81.25% 3 Missing and 3 partials ⚠️
provider/pkg/await/condition/custom.go 80.95% 4 Missing ⚠️
provider/pkg/await/condition/jsonpath.go 86.20% 4 Missing ⚠️
provider/pkg/await/condition/kubectl.go 94.44% 1 Missing and 1 partial ⚠️
provider/pkg/jsonpath/jsonpath.go 95.23% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3134      +/-   ##
==========================================
+ Coverage   38.15%   39.08%   +0.92%     
==========================================
  Files          77       82       +5     
  Lines        9395     9580     +185     
==========================================
+ Hits         3585     3744     +159     
- Misses       5461     5478      +17     
- Partials      349      358       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@blampe blampe force-pushed the blampe/2996-await-custom branch from 253a623 to 95f05a7 Compare August 1, 2024 21:23
@blampe blampe force-pushed the blampe/2996-await-pr branch from 42e88ad to 6377940 Compare August 2, 2024 00:12
@blampe blampe force-pushed the blampe/2996-await-custom branch from 95f05a7 to 7fd8b8a Compare August 2, 2024 00:12
@blampe blampe force-pushed the blampe/2996-await-pr branch from 6377940 to e0cd689 Compare August 7, 2024 17:10
@blampe blampe force-pushed the blampe/2996-await-custom branch from 0b99bab to 2a279fa Compare August 7, 2024 17:11
@blampe blampe force-pushed the blampe/2996-await-pr branch from e0cd689 to eaa802e Compare August 9, 2024 22:14
@blampe blampe force-pushed the blampe/2996-await-custom branch from 2a279fa to bcde5e3 Compare August 9, 2024 22:14
@blampe blampe force-pushed the blampe/2996-await-pr branch from eaa802e to 9a5a997 Compare August 9, 2024 22:52
@blampe blampe force-pushed the blampe/2996-await-custom branch from bcde5e3 to cc1a865 Compare August 9, 2024 22:52
@mjeffryes mjeffryes added this to the 0.108 milestone Aug 16, 2024
@blampe blampe force-pushed the blampe/2996-await-pr branch from 9a5a997 to 03f4cbf Compare August 19, 2024 21:51
@blampe blampe force-pushed the blampe/2996-await-custom branch from cc1a865 to 6dc21ca Compare August 19, 2024 21:52
@blampe blampe force-pushed the blampe/2996-await-pr branch from 03f4cbf to 7bfe53b Compare August 19, 2024 23:23
@blampe blampe force-pushed the blampe/2996-await-custom branch from 6dc21ca to 46c1425 Compare August 19, 2024 23:23
@blampe blampe force-pushed the blampe/2996-await-pr branch from 7bfe53b to e9c82bb Compare August 20, 2024 17:22
@blampe blampe force-pushed the blampe/2996-await-custom branch from 46c1425 to d0e38a1 Compare August 20, 2024 17:22
CHANGELOG.md Outdated
`pulumi.com/waitFor: "jsonpath={.phase}=Running"`

If a value is not provided, the resource will be considered ready when
any value exists at the given path. This resource will wait until it has
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean any non-empty value? Would an empty string be considered a value?

Copy link
Member

Choose a reason for hiding this comment

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

From the code, it looks like an empty string as a value would also be considered ready since it always returns true if the path exists.

// Matches returns true if the JSONPath matches against the given object. If
// the JSONPath didn't include a value, then this will return true if the path
// exists. Otherwise, the path must exist and hold a value equal to the
// Instance's expected value.

@blampe Can we add an additional test case to provider/pkg/await/condition/jsonpath_test.go to confirm/clarify this behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rquitales for sure! FWIW this is based on https://github.com/kubernetes/kubectl/blob/c4be63c54b7188502c1a63bb884a0b05fac51ebd/pkg/cmd/wait/json.go#L72-L91 which succeeds if the path exists with any value.

@mikhailshilkov mikhailshilkov modified the milestones: 0.108, 0.109 Aug 21, 2024
provider/pkg/await/condition/all.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
`pulumi.com/waitFor: "jsonpath={.phase}=Running"`

If a value is not provided, the resource will be considered ready when
any value exists at the given path. This resource will wait until it has
Copy link
Member

Choose a reason for hiding this comment

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

From the code, it looks like an empty string as a value would also be considered ready since it always returns true if the path exists.

// Matches returns true if the JSONPath matches against the given object. If
// the JSONPath didn't include a value, then this will return true if the path
// exists. Otherwise, the path must exist and hold a value equal to the
// Instance's expected value.

@blampe Can we add an additional test case to provider/pkg/await/condition/jsonpath_test.go to confirm/clarify this behaviour?

@blampe blampe force-pushed the blampe/2996-await-pr branch from e9c82bb to 99c297e Compare August 21, 2024 21:30
Base automatically changed from blampe/2996-await-pr to master August 21, 2024 22:07
@blampe blampe force-pushed the blampe/2996-await-custom branch from d0e38a1 to 41bd8fe Compare August 21, 2024 22:30
@blampe blampe enabled auto-merge (squash) August 21, 2024 22:38
@blampe blampe merged commit 48eb571 into master Aug 21, 2024
19 checks passed
@blampe blampe deleted the blampe/2996-await-custom branch August 21, 2024 23:10
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.

Support user-specified readiness/await logic
5 participants