-
Notifications
You must be signed in to change notification settings - Fork 191
Update multi-environment theme command behaviour #6124
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
Conversation
d8cff92
to
d14260f
Compare
Coverage report
Show files with reduced coverage 🔻
Test suite run success3052 tests passing in 1315 suites. Report generated by 🧪jest coverage report action from b382a89 |
/snapit |
🫰✨ Thanks @dejmedus! Your snapshot has been published to npm. Test the snapshot by installing your package globally: pnpm i -g @shopify/[email protected] Tip If you get an Caution After installing, validate the version by running just |
@@ -41,6 +47,7 @@ export default abstract class ThemeCommand extends Command { | |||
async command( | |||
_flags: FlagValues, | |||
_session: AdminSession, | |||
_multiEnvironment = false, |
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 will be helpful in certain commands to toggle off behaviour like individual confirmations
try { | ||
await this.command(flags, session, true, {stdout, stderr}) | ||
|
||
// eslint-disable-next-line no-catch-all/no-catch-all |
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.
Without catching/handling, some command errors (like authentication failures) would prevent subsequent commands from running
20c1ea0
to
7be6f14
Compare
/snapit |
🫰✨ Thanks @graygilmore! Your snapshot has been published to npm. Test the snapshot by installing your package globally: pnpm i -g @shopify/[email protected] Tip If you get an Caution After installing, validate the version by running just |
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.
🎩 went great!
@@ -60,72 +61,125 @@ export default abstract class ThemeCommand extends Command { | |||
|
|||
// Single environment or no environment | |||
if (environments.length <= 1) { | |||
const session = await this.ensureAuthenticated(flags) | |||
const session = await this.createSession(flags) |
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.
Nit: in the future it'd be good to split a name change into a separate commit for an easier git diff!
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.
Sounds good! So its easier to differentiate "cosmetic" changes from actual logic changes/additions?
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.
Exactly!
const environmentsMap = await this.loadEnvironments(environments, flags) | ||
const validationResults = await this.validateEnvironments(environmentsMap, requiredFlags) | ||
|
||
const commandAllowsForceFlag = 'force' in (this.constructor as typeof ThemeCommand)?.flags |
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.
Could we use klass
here since we already type cast on L60?
const commandAllowsForceFlag = 'force' in (this.constructor as typeof ThemeCommand)?.flags | |
const commandAllowsForceFlag = 'force' in klass.flags |
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.
100%, thanks!
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.
Nice changes! 🎩 went well too. I think we're headed in a good direction with this.
Previously required multi-environment command flags were only taken from the `shopify.theme.toml` file. This commit updates ThemeCommand to accept flags from the CLI as well, with CLI flags taking precedence.
Previously multi-environment theme commands did not prompt for confirmation before running. This commit adds a confirmation prompt that will be displayed if the command accepts `--force` flag and has not been set to force.
WHY are these changes introduced?
Previously multi-environment theme commands required
force = "true"
to be set inshopify.theme.toml
since multi env commands must run without interactions. This PR allows flags to be provided via the CLI and adds a confirmation prompt that will run prior to concurrent command calls.WHAT is this pull request doing?
shopify.theme.toml
and the CLI, with CLI flags taking precedence.delete
,push
,pull
) and--force
has not been set.renderConcurrent
to prefix environment name to the error message and ensure that subsequent environment's commands will still be called.multi-environment-demo.mov
How to test your changes?
gh checkout 6124
shopify.theme.toml
fileExample toml
info
andlist
commands both currently work with multiple envs--force
(and confirmation prompts arent shown on commands that dont accept--force
) the force flag can be manually added to test confirmation promptpassword = "fake"
) should have the environment name prefixed to the error message. Commands should fail gracefully and not prevent commands from running in other environments.Post-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist