-
Notifications
You must be signed in to change notification settings - Fork 24
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
Assigns Viridian as the default coordinator for cloud invocations [API-1556] #145
Conversation
✅ Deploy Preview for eclectic-sawine-19fcf1 canceled.
|
run.go
Outdated
return err | ||
} | ||
|
||
func setDefaultCoordinator(logger *log.Logger) { |
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.
Why does this function get a logger as parameter?
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.
It logs if it can not assign the default URL. Do you suggest an alternative flow?
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.
Maybe you can just return the error. Since if the coordinator URL cannot be set we shouldn't continue to run the command anyway.
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 for many usecases this may be non-critical. For those cases just logging makes sense for me. Wdyt?
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 it's a serious problem if the user intends to connect to Viridan but it can't since the coordinator URL was not set correctly. What we can do is to call setDefaultCoordinator
only when we know the user wants to connect to the cloud.
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.
Good idea, updated as discussed e51609e
@utku-caglayan Could you also change:
to:
|
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.
Looks good!
No description provided.