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

Improve Azure deployment reconciler and add tests #1605

Merged
merged 1 commit into from
Jul 2, 2021

Conversation

matthchr
Copy link
Member

What this PR does / why we need it:

  • Fixes a number of issues with the Azure deployment reconciler.
  • Adds various edge-case tests to ensure that scenarios such as creating
    a resource before it's owner is created, or updating a failed resource, work
    correctly.

How does this PR make you feel:
gif

If applicable:

  • this PR contains documentation
  • this PR contains tests

@matthchr
Copy link
Member Author

Ignore the first commit (based on my VMSS branch, will go away once that merges).

This is a draft PR just so folks are aware of the upcoming tests and changes. I think after this we should have enough test coverage to be more confident about the reconciler behavior.

Thinking we can follow this PR up with some refactoring around:

  1. Resource states
  2. Code structure in the reconciler

@matthchr matthchr added this to the codegen-alpha-0 milestone Jun 28, 2021
@matthchr matthchr force-pushed the feature/reconciler-improvements branch from f226466 to 31dff27 Compare June 29, 2021 00:04
@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2021

Codecov Report

Merging #1605 (b5b0021) into master (0b5e1f2) will increase coverage by 0.53%.
The diff coverage is 74.54%.

❗ Current head b5b0021 differs from pull request most recent head 12d7d54. Consider uploading reports for the commit 12d7d54 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1605      +/-   ##
==========================================
+ Coverage   62.38%   62.91%   +0.53%     
==========================================
  Files         186      187       +1     
  Lines       12273    12355      +82     
==========================================
+ Hits         7656     7773     +117     
+ Misses       3926     3898      -28     
+ Partials      691      684       -7     
Impacted Files Coverage Δ
...d/pkg/testcommon/error_translating_roundtripper.go 10.00% <0.00%> (-3.34%) ⬇️
hack/generated/pkg/armclient/types.go 78.02% <33.33%> (+4.15%) ⬆️
...generated/pkg/testcommon/be_provisioned_matcher.go 22.22% <60.00%> (+2.99%) ⬆️
hack/generated/pkg/testcommon/ensure.go 46.87% <66.66%> (+0.20%) ⬆️
...ted/pkg/reconcilers/azure_deployment_reconciler.go 66.58% <81.25%> (+10.50%) ⬆️
...ted/pkg/reconcilers/deployment_error_classifier.go 88.88% <88.88%> (ø)
...ck/generated/pkg/reflecthelpers/reflect_helpers.go 62.88% <100.00%> (+2.06%) ⬆️
hack/generated/pkg/testcommon/kube_matcher.go 100.00% <100.00%> (ø)
.../generated/pkg/testcommon/kube_per_test_context.go 80.48% <100.00%> (+0.82%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b5e1f2...12d7d54. Read the comment docs.

@matthchr matthchr force-pushed the feature/reconciler-improvements branch from 31dff27 to 12d7d54 Compare June 29, 2021 17:55
@matthchr matthchr marked this pull request as ready for review June 29, 2021 17:56
Copy link
Member

@theunrepentantgeek theunrepentantgeek left a comment

Choose a reason for hiding this comment

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

Some minor suggestions, but otherwise looks good.

@@ -31,8 +31,8 @@ func NewEnsure(c client.Client, stateAnnotation string, errorAnnotation string)
}
}

// Provisioned checks to ensure the provisioning state of the resource is successful.
func (e *Ensure) Provisioned(ctx context.Context, obj runtime.Object) (bool, error) {
// State checks to ensure the provisioning state of the resource the target state.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// State checks to ensure the provisioning state of the resource the target state.
// State checks to check whether the provisioning state of the resource has the target state.

"Ensure" is an active word that implies this method will make any changes required for the resource to be in target state; I don't think that's what you mean.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe call the method HasState()?

Copy link
Member Author

@matthchr matthchr Jul 1, 2021

Choose a reason for hiding this comment

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

"Ensure" is an active word that implies this method will make any changes required for the resource to be in target state; I don't think that's what you mean.

The actual struct is already called Ensure - but the methods hanging off as you note do not actually make changes, they just check them. This same comment style is repeated for the other methods Provisioned, Failed, etc.

Merriam-Webster says:

Ensure: to make sure, certain, or safe : guarantee

whereas Cambridge dictionary agrees with you:

To make something certain to happen

I'm going to hold off making a change on this - if we do want to change it we should change the whole class, not just this one place, and it probably ought to be done in a separate PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to HasState

Copy link
Member Author

Choose a reason for hiding this comment

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

I made a note to change Ensure to something else, maybe Verify? Will do it when I have some spare time.

@matthchr matthchr force-pushed the feature/reconciler-improvements branch from 12d7d54 to df71532 Compare July 1, 2021 18:26
  - Add tests ensuring various edge cases are covered.
  - Fix bugs in deployment process which tests uncovered.
@matthchr matthchr force-pushed the feature/reconciler-improvements branch from df71532 to b7987fb Compare July 1, 2021 18:36
@matthchr matthchr merged commit 9aa558c into Azure:master Jul 2, 2021
@matthchr matthchr deleted the feature/reconciler-improvements branch July 2, 2021 00:03
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.

4 participants