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

HCP: error handling & empty source image id #90

Merged
merged 2 commits into from
Dec 16, 2021
Merged

Conversation

azr
Copy link
Contributor

@azr azr commented Nov 23, 2021

No description provided.

@azr azr requested a review from a team as a code owner November 23, 2021 14:23
Comment on lines -134 to -136
if img == nil {
return errors.New("no go on empty image")
}
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 think this was added to avoid a panic in case FromMappedData failed and returned a nil image. In this case, I think that this should panic, since the error was not checked. Also, this error is kind of weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(ditto throughout)

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'm also super open to suggestions!)

Copy link
Contributor

Choose a reason for hiding this comment

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

The error is weird but panicking too. Maybe improving the error message is better 🤔 what do you think? Making it a friendly panic 😆

@azr azr force-pushed the azr/empty-source-image branch from 07ddb72 to 52e55e7 Compare November 23, 2021 14:29
Copy link
Contributor Author

@azr azr left a comment

Choose a reason for hiding this comment

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

In a 1:1 zoom with @nywilken, we are going to merge this one.

It is okay if SourceImageID is not set, and we can revisit that when we work on ancestry.

It's okay to panic when img is nil after an instantiation call failed.

@azr azr merged commit 8aca876 into main Dec 16, 2021
@azr azr deleted the azr/empty-source-image branch December 16, 2021 16:38
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