Skip to content

Conversation

schoren
Copy link
Contributor

@schoren schoren commented May 23, 2023

This PR fixes environment upsert in cli

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

return result, err
if envResource.Spec.GetId() != "" {
_, err := environment.Get(ctx, envResource.Spec.GetId())
if err != nil {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that

Copy link
Contributor

@danielbdias danielbdias left a 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?

@schoren schoren force-pushed the fix-environments-cli branch from 1743d70 to d1e576a Compare May 23, 2023 18:50
@schoren schoren force-pushed the fix-environments-cli branch from d1e576a to b0f75af Compare May 23, 2023 20:19
@danielbdias danielbdias self-requested a review May 23, 2023 21:05
@schoren schoren merged commit e1b4bef into main May 24, 2023
@schoren schoren deleted the fix-environments-cli branch May 24, 2023 16:36
schoren added a commit that referenced this pull request Jun 5, 2023
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.

environment file is not correctly processed when running test on CLI
2 participants