-
Notifications
You must be signed in to change notification settings - Fork 170
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
ARO-3536: Add error handling when "CLUSTER" env variable is empty. #3007
ARO-3536: Add error handling when "CLUSTER" env variable is empty. #3007
Conversation
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.
@edisonLcardenas Good work so far 😃
Just a couple of suggestions to rehuse the variable name that is already defined + proposing to add the same validation in the package cluster to avoid future similar mistakes when using cluster.Cluster from somewhere else.
hack/cluster/cluster.go
Outdated
if clusterName == "" { | ||
return fmt.Errorf("your environment variable \"CLUSTER\" is empty, please pass a value") | ||
} | ||
|
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 think we can use the already defined variable name Cluster instead of hardcoding it on the error message, I would also change a bit the error message, to something like
if clusterName == "" {
return fmt.Errorf("the environment variable %v is empty, please use a non empty value", Cluster)
}
I also think it would be a good idea to add a similar check in pkg/util/cluster/cluster.go:144 as this funcitonality can be potentially called from other places than the hack script as it is a separate component of a different package.
Something like (pkg/util/cluster/cluster.go:143):
func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName string, osClusterVersion string) error {
if clusterName == "" {
return fmt.Errorf("clusterName can not be empty")
}
. . .
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.
how do you feel like move all validations to one place to ValidateVars(), like:
if v, found := os.LookupEnv(v); !found || v == "" {
return fmt.Errorf("environment variable %q unset or set to empty", v)
}
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.
This is a good idea If we are sure that we are not changing the behavior of the code already using ValidateVars().
😃
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.
Thanks for the feedback, I'll take this into consideration and make some updates.
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.
Update pushed. I tested the hack script and creation proceeded after doing the update.
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.
LGTM
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.
LGTM.Thanks! 😃
One last comment as an optional suggestion: I still think that adding also the validation inside pkg/util/cluster/cluster.go:143 in
func (c *Cluster) Create
is a good idea as cluster.Create() could probably be called from somewhere else not touching env vars.
pkg/env/env.go
Outdated
@@ -124,7 +124,7 @@ func IsCI() bool { | |||
// Otherwise it returns nil. | |||
func ValidateVars(vars ...string) error { | |||
for _, v := range vars { | |||
if _, found := os.LookupEnv(v); !found { | |||
if v, found := os.LookupEnv(v); !found || v == "" { |
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.
reuse the variable "v" here does not sound very good, sometimes it is dangerous and hard to debug, it have no problem in the case tho. it does not help for a good coding practice, how about change to another one?
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.
oh, it does have problem here, in case the env variable is set to "", the error handling cannot print out the actual variable name, instead it only prints out an "" as v has been re-assigned.
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.
Yeah, the error handling prints out
FATA[2023-07-20T18:23:54+12:00]hack/cluster/cluster.go:73 main.main() environment variable "" unset
I'll revise this
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 have now updated this. Variable name is appearing now.
[ecardena@ed-thinkpad ARO-RP]$ CLUSTER=$CLUSTER go run ./hack/cluster create
FATA[2023-07-24T17:45:58+12:00]hack/cluster/cluster.go:73 main.main() environment variable "CLUSTER" unset
exit status 1
56e813f
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.
LGTM
/azp run ci |
Azure Pipelines successfully started running 1 pipeline(s). |
Which issue this PR addresses:
https://issues.redhat.com/browse/ARO-3536
Fixes
What this PR does / why we need it:
Error handling when env variable "CLUSTER" is blank.
Running hack create script throws a mem issue if it's just an empty "CLUSTER" env variable.
Test plan for issue:
Run hack create script.
Is there any documentation that needs to be updated for this PR?
No