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

Allow specifying an environment to use variable from #883

Merged
merged 1 commit into from
May 19, 2023

Conversation

graygilmore
Copy link
Contributor

@graygilmore graygilmore commented May 12, 2023

Blocked

Currently based off of the work done here:

WHY are these changes introduced?

A follow up to both #861 and #809 that allows developers to specify which environment to use.

When these commands were first added/updated we defaulted to always pulling the Preview environment variables but there are cases where developers may want to use a different environment for testing purposes.

WHAT is this pull request doing?

We recently shipped the ability to specify a branchName argument in the Admin API for the environmentVariables field. This PR hooks into that field and adds a new flag to commonFlags to facilitate it.

HOW to test your changes?

  1. Pull the branch and navigate to the cli directory
  2. Run npm run build
  3. Navigate to the demo store directory cd ../../templates/demo-store
  4. Run h2 env pull --branch <BRANCH> (note: if h2 is unavailable run npx shopify hydrogen shortcut first) to pull your specified environment variables down
    • You should be prompted to provide a shop
    • You should then be prompted to authenticate with that shop
    • You should be prompted that you need to link to a storefront
    • Note: replace <BRANCH> with the branch associated to an environment on your linked shop (e.g. main for most Production environments)
  5. Confirm that the variables pulled are from your specified branch's environment

Checklist

  • I've read the Contributing Guidelines
  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've added a changeset if this PR contains user-facing or noteworthy changes
  • I've added tests to cover my changes
  • I've added or updated the documentation

@graygilmore graygilmore changed the title Allow specifying an environment for variables Allow specifying an environment to use variable from May 15, 2023
@graygilmore graygilmore marked this pull request as ready for review May 15, 2023 17:14
@graygilmore graygilmore requested a review from frandiox May 15, 2023 17:14
@graygilmore graygilmore mentioned this pull request May 15, 2023
5 tasks
Copy link
Contributor

@frandiox frandiox left a comment

Choose a reason for hiding this comment

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

Thanks! Maybe we should wrap the envPull call and improve error messages, since we know the response format:

image

For example, in the previous situation we could render a human readable "environment not found" error message and mention what they can try to fix the issue (use the "env list" command?).


Also, is "Preview" in the following correct? I think this is our Production branch.

image

packages/cli/src/lib/flags.ts Outdated Show resolved Hide resolved
packages/cli/src/commands/hydrogen/dev.ts Outdated Show resolved Hide resolved
@graygilmore graygilmore force-pushed the automatically-inject-variables branch 3 times, most recently from ecb0335 to 845007e Compare May 17, 2023 21:53
@graygilmore
Copy link
Contributor Author

Force Push Patch Notes

  • Rebased with automatically-inject-variables since this PR is based on that work
    • Needed to make a small change now that combinedEnvironmentVariables is in a separate file

@graygilmore
Copy link
Contributor Author

graygilmore commented May 18, 2023

Force Push Patch Notes

  • Renamed branch flag to env-branch
  • Marked the flag as hidden
  • Fixed a bug where we were still hardcoding Preview as the environment

@graygilmore
Copy link
Contributor Author

Maybe we should wrap the envPull call and improve error messages, since we know the response format

I remember looking into this in my original exploration of these commands because we were thinking about returning custom error codes. Unfortunately (or fortunately depending on your perspective!) cli-kit captures the error and returns the output that you see in the screenshot. We'd have to overwrite how it does admin requests in order for us to capture the error and render it ourselves which I'd like to avoid doing. Let me know what you think!

@frandiox
Copy link
Contributor

I remember looking into this in my original exploration of these commands because we were thinking about returning custom error codes. Unfortunately (or fortunately depending on your perspective!) cli-kit captures the error and returns the output that you see in the screenshot. We'd have to overwrite how it does admin requests in order for us to capture the error and render it ourselves which I'd like to avoid doing. Let me know what you think!

It looks like we can actually pass responseOptions: {handleErrors: false}, to graphqlRequest and wrap it in try/catch. This allows custom error handling on our end.

However, feel free to merge this without that change, it's something we can improve later 👍

Base automatically changed from automatically-inject-variables to 2023-04 May 19, 2023 15:35
@graygilmore
Copy link
Contributor Author

Force Push Patch Notes

@graygilmore graygilmore merged commit 24b82fc into 2023-04 May 19, 2023
@graygilmore graygilmore deleted the specify-env branch May 19, 2023 15:46
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