-
Notifications
You must be signed in to change notification settings - Fork 606
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 traffic splitter API kind #1213
Conversation
pkg/operator/resources/resources.go
Outdated
return nil, err | ||
} | ||
} else if deployedResource.Kind == userconfig.APISplitterKind { | ||
if deployedResource == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the comment above, this check can be removed once the if deployedResource == nil
check is made before the kind specific deletions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see other comment.
|
||
return nil, ErrorAPINotDeployed(apiName) | ||
} | ||
|
||
if deployedResource.Kind == userconfig.SyncAPIKind { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going back and forth on this one. As we add more kinds, this delete will get more expensive (and potentially more redundant). However, I do like the idea that if for some there is an error in the delete and the virtual service is deleted but there other dangling resources (e.g. api gateway), we can delete again to try to cleanup again.
We should bring it back to help the users cleanup resources as much as possible.
if err := isWeight100(api.APIs); err != nil { | ||
return errors.Wrap(err, api.Identify(), userconfig.PredictorKey) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like we validate that the names listed in api.APIs
are unique, we should validate add that validation here.
@@ -147,24 +156,45 @@ func DeleteAPI(apiName string, keepCache bool) (*schema.DeleteResponse, error) { | |||
return nil, err | |||
} | |||
|
|||
if deployedResource == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the conversation, the "just to be sure" deletion can be brought back. Also add api splitter to the just to be sure deletion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it back already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nil check should happen before if deployedResource.Kind == userconfig.SyncAPIKind {
is called because you may run into a nil pointer exception if deployedResource is nil.
closes #1132, closes #1089, closes #275
checklist:
make test
andmake lint
<--- some listing issue atmsummary.md
(view in gitbook after merging)