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

Add support for Compute/Image and Compute/Snapshot #2090

Merged
merged 17 commits into from
Feb 15, 2022

Conversation

theunrepentantgeek
Copy link
Member

What this PR does / why we need it:

Adds support for two new resources - Compute/Image and Compute/Snapshot.

Closes #2057

How does this PR make you feel:
gif

If applicable:

  • this PR contains tests

@codecov-commenter
Copy link

Codecov Report

Merging #2090 (3ab5639) into main (d995cb4) will decrease coverage by 0.03%.
The diff coverage is 56.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2090      +/-   ##
==========================================
- Coverage   56.33%   56.30%   -0.04%     
==========================================
  Files         554      562       +8     
  Lines      103002   105195    +2193     
==========================================
+ Hits        58027    59227    +1200     
- Misses      37786    38581     +795     
- Partials     7189     7387     +198     
Impacted Files Coverage Δ
...ompute/customizations/image_extension_types_gen.go 0.00% <0.00%> (ø)
...ute/customizations/snapshot_extension_types_gen.go 0.00% <0.00%> (ø)
...api/compute/v1alpha1api20210701/image_types_gen.go 52.38% <ø> (ø)
...alpha1api20200930/snapshots__spec_arm_types_gen.go 33.33% <33.33%> (ø)
.../v1alpha1api20210701/images__spec_arm_types_gen.go 33.33% <33.33%> (ø)
...ha1api20201201storage/virtual_machine_types_gen.go 55.69% <54.00%> (-2.93%) ⬇️
.../compute/v1alpha1api20200930/snapshot_types_gen.go 56.91% <56.91%> (ø)
...e/v1alpha1api20200930storage/snapshot_types_gen.go 58.62% <58.62%> (ø)
...pute/v1alpha1api20210701storage/image_types_gen.go 58.62% <58.62%> (ø)
...2/internal/controllers/controller_resources_gen.go 86.17% <88.88%> (+0.07%) ⬆️
... and 4 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 d995cb4...3ab5639. Read the comment docs.

Taskfile.yml Outdated
@@ -252,7 +252,11 @@ tasks:
deps: [controller:run-kustomize-for-envtest]
cmds:
# -race fails at the moment in controller-runtime
- go test -timeout 15m -run '{{default ".*" .TEST_FILTER}}' ./internal/controllers

Copy link
Member

Choose a reason for hiding this comment

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

minor: Extra newline here that looks unneeded?

v2/config/samples/compute/v1alpha1api20210701_image.yaml Outdated Show resolved Hide resolved
tc.CreateResourceAndWait(image)
tc.Expect(image.Status.Id).ToNot(BeNil())
imageARMId := *image.Status.Id

Copy link
Member

Choose a reason for hiding this comment

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

minor: Most tests we try to do a patch/update section too (just to ensure patch/update works and catch any possible issues in the RP like what @babbageclunk found where they didn't accept PUT for update). See for example the crd_networking_publicip_test.go

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 didn't find any properties that could be patched after the initial create.

Copy link
Member

@matthchr matthchr Feb 15, 2022

Choose a reason for hiding this comment

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

You can almost always patch Tags, but it's not all that interesting since we use that trick in lots of other places already so it's unlikely it's going to be broken for just this resource. I'm fine with no Patch here.

}

tc.CreateResourceAndWait(snapshot)
tc.Expect(snapshot.Status.Id).ToNot(BeNil())
Copy link
Member

Choose a reason for hiding this comment

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

Same comment, assert on a few key properties (disk size?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added asserts.

@theunrepentantgeek theunrepentantgeek deleted the feature/compute-images branch February 15, 2022 22:24
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.

Feature: Add support for Microsoft.Compute/images and Microsoft.Compute/snapshots
3 participants