Skip to content
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

Add base infrastructure and first command for multi-environment commands #5284

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

EvilGenius13
Copy link
Contributor

WHY are these changes introduced?

Developers working on multiple stores keep a shopify.toml file that can contain data for multiple stores such as themes, passwords etc. Any theme command they run would have to be run separately for each store they want to access.

WHAT is this pull request doing?

This PR introduces the framework for multi environment support. the environment flag now accepts an array instead of a string allowing us to parse multiple stores at once. A new multi-run function handles commands dynamically, and allows them to run concurrently.

This PR is shipping with the theme list command available and follow up PR's will add more commands.

Example theme list command with single environment:

image

Example theme list command with multiple environments:

image

How to test your changes?

  • Pull down the branch
  • Build the branch
  • If you don't already have one, create a shopify.theme.toml file

Note: You will need to create an access token through your shop(s) to bypass the need to login during theme commands.
The most basic example of the structure needed would be this:

[environments.env1]
store = "myfirstshop.myshopify.com"
password = "access_token"
[environments.env2]
store = "mysecondshop.myshopify.com"
password = "access_token"
  • Run theme list -e env1 -e env2 or whatever you named your environments.

  • Look for all your stores themes to be rendered on your terminal. The order may differ from your input (i.e you might have put env1, env2 but the output is env2, env1) due to the commands running concurrently.

Post-release steps

Post release steps will mostly revolve around follow up PR's for the other commands.

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

@EvilGenius13 EvilGenius13 requested review from a team as code owners January 27, 2025 20:56

This comment has been minimized.

@EvilGenius13 EvilGenius13 requested a review from karreiro January 27, 2025 20:56
Copy link
Contributor

github-actions bot commented Jan 27, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.46% (-0% 🔻)
8996/11921
🟡 Branches
70.64% (-0.01% 🔻)
4388/6212
🟡 Functions 75.29% 2362/3137
🟡 Lines
75.98% (+0.01% 🔼)
8498/11184
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
95.18% (-1.2% 🔻)
86.49% (-2.7% 🔻)
95.45% 100%
🟢
... / base-command.ts
83.95% (-0.86% 🔻)
78.85% (-1.15% 🔻)
77.27%
85.71% (+0.21% 🔼)

Test suite run success

2033 tests passing in 909 suites.

Report generated by 🧪jest coverage report action from b2ab0f9

const {flags} = await this.parse(List)
const store = ensureThemeStore(flags)
const adminSession = await ensureAuthenticatedThemes(store, flags.password)
static multiEnvironmentsFlags = ['store', 'password']
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These flags are the necessary flags the toml file would need to run. For instance, in a push or pull theme, you would be adding the theme flag to this array.

@EvilGenius13 EvilGenius13 force-pushed the multi-env-infrastructure branch 2 times, most recently from 45a5994 to 5b8453d Compare January 29, 2025 18:26
Copy link
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this PR, @EvilGenius13! I've left only one minor comment regarding how we present each environment.

Also, I've noticed that if I remove the password flag, we get this message (with the password flag duplicated):

image

Thanks again for this PR, @EvilGenius13 🔥 🚀

@@ -31,6 +31,8 @@ export async function list(adminSession: AdminSession, options: Options) {
storeThemes = filterThemes(store, storeThemes, filter)
}

outputInfo(`Store: ${store}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we could break a line here and show the environment as an user input, as we may have multiple environments in the same store.

@EvilGenius13
Copy link
Contributor Author

Thanks a lot for this PR, @EvilGenius13! I've left only one minor comment regarding how we present each environment.

Also, I've noticed that if I remove the password flag, we get this message (with the password flag duplicated):

image

Thanks again for this PR, @EvilGenius13 🔥 🚀

I added the environment to the flag message so instead of only showing the error once, we state which environment(s) it's missing from

image
OR
image

This commit adds the groundwork to use commands when passing multiple
environments. We are starting with the list command
@EvilGenius13 EvilGenius13 force-pushed the multi-env-infrastructure branch from c1be9b1 to c6205cf Compare January 30, 2025 18:45
@EvilGenius13 EvilGenius13 force-pushed the multi-env-infrastructure branch from ca508cf to b2ab0f9 Compare January 30, 2025 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants