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

Fixing Read failure of 404 from Pulumi Service #313

Merged
merged 1 commit into from
Jun 15, 2024

Conversation

IaroslavTitov
Copy link
Contributor

Summary

  • The crux of the issues turned out that ESC client lumps in 404 with other errors
  • So we need to NOT throw error on 404, but instead just return empty response

###Testing

  • Follow repro steps after fix, bug no longer happens

@IaroslavTitov IaroslavTitov marked this pull request as ready for review June 10, 2024 18:32
@@ -163,8 +164,8 @@ func (evt *PulumiServiceEnvironmentVersionTagResource) Read(req *pulumirpc.ReadR
}

tag, err := evt.client.GetEnvironmentRevisionTag(ctx, input.Organization, input.Environment, input.TagName)
if err != nil {
return nil, fmt.Errorf("failed to read StackTag (%q): %w", req.Id, err)
if err != nil && !strings.Contains(err.Error(), "404") {
Copy link
Member

Choose a reason for hiding this comment

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

This is unfortunate! ESC client should really return a rest error here. At its core ESC is just calling the pulumi service so it should return a rest error the way the service does. Did you try using

func GetErrorStatusCode(err error) int {
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checked, and unfortunately this does not work, because this method is trying to cast into errorResponse, but the root of the error returned by the client is just a string created with fmt.Errorf

I can create a helper method to make this check a bit more elegant, but unfortunately don't see a way around checking string for 404

Copy link
Member

Choose a reason for hiding this comment

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

Bummer, thanks for trying it out

@IaroslavTitov IaroslavTitov merged commit bfd8b59 into main Jun 15, 2024
13 checks passed
@IaroslavTitov IaroslavTitov deleted the iaro/tag-preview-bug branch June 15, 2024 00:11
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.

2 participants