-
Notifications
You must be signed in to change notification settings - Fork 288
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
[cli] hydrogen deploy: Add token flag, add environment selection #1381
Conversation
This comment has been minimized.
This comment has been minimized.
acdd4e8
to
caf480e
Compare
caf480e
to
417a63a
Compare
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!
@@ -36,6 +39,11 @@ export const deploymentLogger: Logger = ( | |||
|
|||
export default class Deploy extends Command { | |||
static flags: any = { | |||
environmentTag: Flags.string({ |
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.
Just a suggestion, we are using "env" instead of "environment" in other flags, so maybe this one could be "envTag" instead or even "envBranch" like in h2 env pull
?
Also, just noticed these flags are using camel case but we use kebab case for flags everywhere else. I don't think Oclif transform them internally so we should write them as kebab case here.
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 converted all to kebab case now. Are existing flags like envBranch
or other commonFlags changing too?
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.
Ty! The commonFlags
object can use camel case because it's internal, but the flags are later exposed as kebab-case already afaik -- if we missed some flag we should change it, yeah
environmentTag: flags['env-branch'], | ||
path: actualPath, | ||
shop: flags.shop, | ||
publicDeployment: flags.publicDeployment, | ||
publicDeployment: flags['public-deployment'], | ||
token: flags.token, | ||
metadataUrl: flags.metadataUrl, | ||
metadataUser: flags.metadataUser, | ||
metadataVersion: flags.metadataVersion, | ||
metadataUrl: flags['metadata-url'], | ||
metadataUser: flags['metadata-user'], | ||
metadataVersion: flags['metadata-version'], |
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.
FYI you can use the flagsToCamelObject
utility, it's also typed. You can see an example in commands/dev.ts and other commands
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.
Neat, thanks for sharing.
f55873d
to
9325faf
Compare
WHY are these changes introduced?
token
flag to allow manual token entryoxygen-cli
2.0.0 which renames the health check to "deployment verification".WHAT is this pull request doing?
This will enable merchants to use the
deploy
command in CI environments, where the automatic token retrieval through the API will not be available. Instead, they can use thetoken
flag to supply an Oxygen deployment token.In addition; outside of CI environments we can display a user-friendly list of store environments to deploy to, with the current default based on the currently checked-out branch.
HOW to test your changes?
Post-merge steps
Checklist