feat(apps): Let users deploy to their own docker hub (and other fixes) #277
Conversation
cmd/meroxa/root/apps/deploy.go
Outdated
| @@ -44,15 +49,17 @@ type createApplicationClient interface { | |||
|
|
|||
| type Deploy struct { | |||
| flags struct { | |||
| Path string `long:"path" description:"path to the app directory"` | |||
| Path string `long:"path" description:"path to the app directory (default is local directory)"` | |||
| DockerHubUserName string `long:"docker-hub-username" description:"DockerHub username to use to build and deploy the application image"` | |||
There was a problem hiding this comment.
This is neat! Do we want to expose to the user the concept of a function image in the descriptions for these flags, too?
Technically, we're only building a function image, instead of an application image, on Docker. What are your thoughts on the wording though and how much the user should be aware about the distinction between deploying an app and building a function image?
There was a problem hiding this comment.
@jayjayjpg I updated the wording last night (mostly due to linting), but now the message is a bit broader. Please take a look and tell me what you think.
| } | ||
|
|
||
| func (d *Deploy) validateLocalDeploymentConfig() error { | ||
| // Check if users had an old configuration where DockerHub credentials were set as environment variables |
There was a problem hiding this comment.
The two step validation step for both environment variables and flags is really neat ✨
| // Check if users had an old configuration where DockerHub credentials were set as environment variables | ||
| err := d.validateDockerHubEnvVars() | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Could we also log out the error here using logger.Error onto the console or is this already taken care of by returning the err variable?
There was a problem hiding this comment.
@jayjayjpg It's been taken care of. That function would return the error to then to the Execute function which Cobra (our CLI. builder, really) will print out as an error.
There was a problem hiding this comment.
Got it and thank you for the outline, that's super helpful!
| default: | ||
| return fmt.Errorf("language %q not supported. %s", d.lang, LanguageNotSupportedError) | ||
| } | ||
|
|
||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
If the deploy should fail with an error, would we need to log the error explicitly here via logger.Error or is this already taken care of by returning the err object?
There was a problem hiding this comment.
if the error is correctly returned by either deploy step, the CLI will ultimately print out the error returned on the execution.
There was a problem hiding this comment.
Got it, I wasn't fully aware yet, that an error essentially needs to be passed through to different commands and that sounds good!
| } | ||
| }) | ||
| } | ||
| } |
| Path string `long:"path" description:"path to the app directory"` | ||
| Path string `long:"path" description:"path to the app directory (default is local directory)"` | ||
| DockerHubUserName string `long:"docker-hub-username" description:"DockerHub username to use to build and deploy the app"` | ||
| DockerHubAccessToken string `long:"docker-hub-access-token" description:"DockerHub access token to use to build and deploy the app"` |
There was a problem hiding this comment.
Yes, this reads great and thank you for the swift update! ✨
| // Check if users had an old configuration where DockerHub credentials were set as environment variables | ||
| err := d.validateDockerHubEnvVars() | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Got it and thank you for the outline, that's super helpful!
| default: | ||
| return fmt.Errorf("language %q not supported. %s", d.lang, LanguageNotSupportedError) | ||
| } | ||
|
|
||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Got it, I wasn't fully aware yet, that an error essentially needs to be passed through to different commands and that sounds good!
apps initfor JS.Type of change
How was this tested?
Demo
See slack.