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

Enable include_config_on_deploy by default for new apps #5253

Merged
merged 3 commits into from
Jan 28, 2025

Conversation

gonzaloriestra
Copy link
Contributor

@gonzaloriestra gonzaloriestra commented Jan 22, 2025

WHY are these changes introduced?

When you create a new app in Partners with shopify app init, the generated TOML should include include_config_on_deploy = true.

It was working some time ago, but then broken when we added the authentication to the app init. The reason is that we were checking if the app is new to add that field, but after that refactor, we create the app first and then fetch it again when running link, so that the newApp property was missed.

WHAT is this pull request doing?

The link function is already receiving a newApp option, so I'm just setting it again to the returned app if required.

How to test your changes?

  • npm i -g @shopify/[email protected]
  • shopify app init
  • Check that the app TOML includes include_config_on_deploy = true
  • shopify app deploy doesn't ask about including the config

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

Copy link
Contributor

github-actions bot commented Jan 22, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.34% (+0% 🔼)
8892/11803
🟡 Branches
70.59% (+0.03% 🔼)
4325/6127
🟡 Functions 75.1% 2337/3112
🟡 Lines
75.87% (+0.01% 🔼)
8403/11075
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
93.83% (-1.23% 🔻)
86.49% 90.48% 98.61%

Test suite run success

2002 tests passing in 904 suites.

Report generated by 🧪jest coverage report action from 63192ff

@gonzaloriestra gonzaloriestra marked this pull request as ready for review January 22, 2025 16:34
@gonzaloriestra gonzaloriestra requested a review from a team as a code owner January 22, 2025 16:34

This comment has been minimized.

@gonzaloriestra gonzaloriestra requested a review from a team as a code owner January 23, 2025 10:13
@gonzaloriestra
Copy link
Contributor Author

/snapit

Copy link
Contributor

🫰✨ Thanks @gonzaloriestra! Your snapshot has been published to npm.

Test the snapshot by intalling your package globally:

pnpm i -g @shopify/[email protected]

After installing, validate the version by running just shopify in your terminal
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

@@ -116,32 +116,31 @@ describe('link', () => {
expect(remoteApp).toEqual(mockRemoteApp({developerPlatformClient}))
})
})
})
Copy link
Contributor

Choose a reason for hiding this comment

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

was this change intentional? seems like it changed the indentation of all tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, most of the tests here were outside of the describe where they should belong. Sorry for the noise 😅

Copy link
Contributor Author

gonzaloriestra commented Jan 28, 2025

Merge activity

  • Jan 28, 10:23 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Jan 28, 10:23 AM EST: A user added this pull request to the GitHub merge queue with Graphite.

@gonzaloriestra gonzaloriestra added this pull request to the merge queue Jan 28, 2025
Merged via the queue into main with commit 253f855 Jan 28, 2025
27 checks passed
@gonzaloriestra gonzaloriestra deleted the include_config_on_deploy_by_default branch January 28, 2025 15:30
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