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

Fix asoctl aborting import when an error occurs #3212

Merged
merged 7 commits into from
Aug 29, 2023

Conversation

theunrepentantgeek
Copy link
Member

What this PR does / why we need it:

It's possible to encounter an error querying ARM for child resources that don't exist.
This currently causes asoctl to abort the import run but shouldn't.

Fixes #3200

Special notes for your reviewer:

Amongst other causes, errors mayoccur if the subscription hasn't onboarded for a particular resource provider, or if the subscription doesn't have access to a particular new feature.

How does this PR make you feel:
gif

@codecov-commenter
Copy link

codecov-commenter commented Aug 28, 2023

Codecov Report

Merging #3212 (cb08cc4) into main (239eea7) will increase coverage by 0.02%.
Report is 1 commits behind head on main.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #3212      +/-   ##
==========================================
+ Coverage   54.39%   54.41%   +0.02%     
==========================================
  Files        1424     1424              
  Lines      601351   601360       +9     
==========================================
+ Hits       327097   327230     +133     
+ Misses     220754   220622     -132     
- Partials    53500    53508       +8     
Files Changed Coverage Δ
...octl/internal/importing/importable_arm_resource.go 2.60% <0.00%> (-0.04%) ⬇️
...d/asoctl/internal/importing/importable_resource.go 37.14% <ø> (ø)
...cmd/asoctl/internal/importing/resource_importer.go 0.00% <0.00%> (ø)

... and 28 files with indirect coverage changes

@theunrepentantgeek theunrepentantgeek changed the title Fix/asoctl resource listing Fix asoctl giving up with an error occurs Aug 28, 2023
@theunrepentantgeek theunrepentantgeek changed the title Fix asoctl giving up with an error occurs Fix asoctl aborting import when an error occurs Aug 28, 2023
Copy link
Member

@matthchr matthchr left a comment

Choose a reason for hiding this comment

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

Lacking a test related to this problem. I see that (like the core logic of the controller) it's somewhat difficult to unit test this code as it stands today. In the controller though we do have the recording tests to fall back on.

Wondering here if we should define an interface that allows us to mock certain errors from Azure for aid in testing these cases? We actually already have one in extensions.ImporterFunc... possibly we could leverage that with some test helpers to make it ergonomic to mock certain error responses from Azure and ensure we handle them?

Approved because I don't necessarily think that should come in this PR, but maybe we should think about it and file an item for it?

@@ -8,6 +8,7 @@ package importing
import (
"context"
"fmt"
kerrors "k8s.io/apimachinery/pkg/util/errors"
Copy link
Member

Choose a reason for hiding this comment

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

minor: Wrong ordering, should be below with the non-stdlib imports.

@@ -94,11 +95,44 @@ func (i *importableARMResource) Resource() genruntime.MetaObject {
// ctx is the context to use for the import.
// Returns a slice of child resources needing to be imported (if any), and/or an error.
Copy link
Member

Choose a reason for hiding this comment

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

minor: fix docs

@theunrepentantgeek theunrepentantgeek enabled auto-merge (squash) August 29, 2023 01:29
@theunrepentantgeek theunrepentantgeek merged commit 55e6ec8 into main Aug 29, 2023
8 checks passed
@theunrepentantgeek theunrepentantgeek deleted the fix/asoctl-resource-listing branch August 29, 2023 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Bug: "400 Bad Request" when importing ManagedClusters with asoctl v2.2.0
4 participants