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

[Feature] Add ability to include liquid in TOML files, pass through liquid renderer before parsing as TOML #4118

Closed
wants to merge 1 commit into from

Conversation

jduff
Copy link

@jduff jduff commented Jun 24, 2024

WHY are these changes introduced?

There is currently no way to template toml file if you want to supply some of the values via environment variables. I believe the recommended approach to handling multiple environments is to have multiple named configuration files. While this works, I still think it is risky to include the working production config in a repo and would rather supply the production key at deploy time in an isolated environment.

WHAT is this pull request doing?

Since this project already uses Liquid I opted to use that as the templating language that can be used within the toml files. This PR passes all toml files through a Liquid renderer providing the environment variables as data to the rendered before parsing the toml file.

How to test your changes?

I have added tests to this PR but I am not able to test them as I can't install the development dependencies. You can also test this PR by using a TOML file with liquid in it and running any of the cli commands.

Example shopify.app.toml:

client_id = "{{ env.SHOPIFY_APP_API_KEY }}"
name = "{{ env.SHOPIFY_APP_NAME }}"
# the rest of the file

Running the cli deploy command should succeed if given valid environment variables ex.

SHOPIFY_APP_API_KEY=key SHOPIFY_APP_NAME=test npm run deploy

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

@nickwesselman
Copy link
Contributor

Hi @jduff! We really appreciate the PR! We have been thinking about adding interpolation support in the config TOMLs, but we're not sure yet what the syntax will be. I'm concerned about compatibility issues if we ship this now.

In the mean time you might consider maintaining a .liquid version of the toml in your repository, and using an npm script which interpolates and runs the desired Shopify CLI command.

@jduff
Copy link
Author

jduff commented Jul 23, 2024

Thanks, ya I've done something similar in the meantime. Just seemed like something simple and useful to include in for everyone so I opened the pr.

Really appreciate you getting back to me and letting me know the reason not to accept it though.

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