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

Don't panic in ParseDriverType #1363

Merged
merged 3 commits into from
Dec 11, 2019

Conversation

SimonAlling
Copy link
Contributor

Description of the change

It rewrites agent.ParseDriverType to return an error instead of panicking if it doesn't recognize its input.

Benefits

  • ParseDriverType is made more faithful to its declared type.
  • It's made more obvious in main.go that the application may panic.

Possible drawbacks

  • ParseDriverType becomes more verbose.

Applicable issues

Excerpt from a related discussion with @andresmgot in #1359:

I'm much more inclined to make agent.ParseDriverType return an error instead of panicking, because, unlike createStorage, it promises that it can handle any string. Its current deceitfulness stems from my interpretation of @absoludity's suggestion that Kubeops should refuse to start if an invalid driver type has been specified. I'll be more than happy to return an error instead.

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

LGTM, I have a couple of small comments, let me know what you think

driverType = agent.ParseDriverType(helmDriverArg)
d, err := agent.ParseDriverType(helmDriverArg)
if err != nil {
panic(err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

you can just do panic(err)

@@ -90,16 +91,16 @@ func createStorage(driverType DriverType, namespace string, clientset *kubernete
return store
}

func ParseDriverType(raw string) DriverType {
func ParseDriverType(raw string) (DriverType, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this function is simple but it'd be good to have a simple test

@SimonAlling
Copy link
Contributor Author

LGTM, I have a couple of small comments, let me know what you think

I have fixed those issues now! 😃

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

pkg/agent/agent_test.go Show resolved Hide resolved
@andresmgot andresmgot merged commit 6e6f10e into vmware-tanzu:master Dec 11, 2019
@SimonAlling SimonAlling deleted the parse-driver-type-error branch December 11, 2019 14: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