-
Notifications
You must be signed in to change notification settings - Fork 70
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(cli): fix upsert for environments #2570
Conversation
return result, err | ||
if envResource.Spec.GetId() != "" { | ||
_, err := environment.Get(ctx, envResource.Spec.GetId()) | ||
if err != 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.
We need to add validation here because we need guarantees that this error happens because of a missing entity.
One way is to emit a specialized error like "ResourceNotFound" to do that. I did this here: 9e9ec20#diff-660f1c4de144c4d56d5c007996cd5725aa2bb25a43caf6cb6f7600424790062fR154
I've added this validation on my CLI e2e branch to print a "Resource not found message" in case of a user asks for an entity that doesn't exist.
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 like that
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 PR sounds fine to solve this problem.
However, we must revisit a minor issue our Get
method has on the CLI of not handling 404 errors well. I've added a comment on that. Does it makes sense?
1743d70
to
d1e576a
Compare
d1e576a
to
b0f75af
Compare
This PR fixes environment upsert in cli
Checklist