Skip to content

Add nested condition block to wait block #1595

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 28, 2022
Merged

Add nested condition block to wait block #1595

merged 3 commits into from
Apr 28, 2022

Conversation

jrhouston
Copy link
Collaborator

@jrhouston jrhouston commented Feb 8, 2022 •
edited
Loading

Description

This PR adds a nested condition block to the wait block to configure sets of conditions to wait to be met.

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

Release Note

Release note for CHANGELOG:

Add nested condition block to wait block

References

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

@jrhouston jrhouston force-pushed the wait-for-conditions branch from a44df61 to b05a178 Compare February 8, 2022 09:20
Base automatically changed from wait-for-rollout to main March 31, 2022 17:04
@jrhouston jrhouston force-pushed the wait-for-conditions branch 5 times, most recently from cd19559 to b7d163e Compare April 11, 2022 21:05
@jrhouston jrhouston requested a review from arybolovlev April 12, 2022 01:45
arybolovlev
arybolovlev approved these changes Apr 13, 2022
View reviewed changes
Copy link
Contributor

@arybolovlev arybolovlev left a comment

Choose a reason for hiding this comment

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

Nice addition! 🚀 Just one minor comment in the documentations section.

website/docs/r/manifest.html.markdown Outdated
@@ -200,6 +200,7 @@ The following arguments are supported:
#### Arguments

- **rollout** (Optional) When set to `true` will wait for the resource to roll out, equivalent to `kubectl rollout status`.
- **conditions** (Optional) A set of conditions to wait for.
Copy link
Contributor

Choose a reason for hiding this comment

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

The conditions block has two nested attributes status and type. Could you please mention them in the documentation too?

One small comment regarding style here. Could you please make all argument names formatted as code instead of bold text?

alexsomesan
alexsomesan reviewed Apr 22, 2022
View reviewed changes
Copy link
Member

@alexsomesan alexsomesan 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!
Small observation where we could simplify logic a bit, otherwise all good stuff!

manifest/provider/waiter.go Outdated
var conditionType, conditionStatus string
condition["type"].As(&conditionType)
condition["status"].As(&conditionStatus)
conditionsMet[conditionType] = false
Copy link
Member

Choose a reason for hiding this comment

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

This could actually just be a boolean that is progressively AND-ed to the current condition status in the loop. This way, it only becomes true when no condition is false. Seems a bit simpler, since we're not reporting the individual conditions status up to the user.

manifest/provider/waiter.go Outdated
if ccc["type"].(string) != conditionType {
continue
}
conditionsMet[conditionType] = ccc["status"].(string) == conditionStatus
Copy link
Member

Choose a reason for hiding this comment

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

AND updating of boolean conditionsMet could happen here.

manifest/provider/waiter.go Outdated
Comment on lines 364 to 369
done := true
for _, v := range conditionsMet {
if !v {
done = false
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This wouldn't be needed when using accumulator boolean. Instead of "done", the check would be on directly "conditionsMet".

@jrhouston jrhouston force-pushed the wait-for-conditions branch from b7d163e to 5b2ffdb Compare April 28, 2022 17:05
@jrhouston jrhouston merged commit 5bdba47 into main Apr 28, 2022
@jrhouston jrhouston deleted the wait-for-conditions branch April 28, 2022 20:43
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alexsomesan alexsomesan alexsomesan left review comments

@arybolovlev arybolovlev arybolovlev approved these changes

Assignees
No one assigned
Labels
documentation size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jrhouston @arybolovlev @alexsomesan