Skip to content

Don't use cached config name when using --reset#5676

Merged
isaacroldan merged 1 commit into
mainfrom
04-22-fix_behavior_of_--reset_not_creating_a_new_toml_file
Apr 25, 2025
Merged

Don't use cached config name when using --reset#5676
isaacroldan merged 1 commit into
mainfrom
04-22-fix_behavior_of_--reset_not_creating_a_new_toml_file

Conversation

@isaacroldan

@isaacroldan isaacroldan commented Apr 22, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

WHY are these changes introduced?

When forcing a relink of an app, the system was still using the cached configuration file name, which made the overwrite the existing config.

Fixes #5653

WHAT is this pull request doing?

Modifies the linkedAppContext function to set the configName to undefined when forceRelink is true, allowing users to be prompted for a new configuration file name instead of using the cached one.

How to test your changes?

  1. Run a command with --reset
  2. Verify that you are prompted to select a configuration file name
  3. Confirm that the new configuration file is used after relinking

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

@isaacroldan Graphite App

isaacroldan commented Apr 22, 2025

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@isaacroldan isaacroldan changed the title Fix behavior of --reset not creating a new toml file Don't use cached config name when using --reset Apr 22, 2025
@isaacroldan isaacroldan marked this pull request as ready for review April 22, 2025 11:16
@isaacroldan isaacroldan requested a review from a team as a code owner April 22, 2025 11:16
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

github-actions Bot commented Apr 22, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Coverage report

St.❔
 Category Percentage Covered / Total
🟡 Statements
76.68% (+0.02% 🔼)
 9481/12364
🟡 Branches
71.81% (+0.05% 🔼)
 4657/6485
🟡 Functions 76.45% 2457/3214
🟡 Lines
77.22% (+0.02% 🔼)
 8967/11612

Test suite run success

2205 tests passing in 959 suites.

Report generated by 🧪jest coverage report action from 748f607

@isaacroldan isaacroldan force-pushed the 04-22-fix_behavior_of_--reset_not_creating_a_new_toml_file branch from 9791370 to 748f607 Compare April 22, 2025 12:05
@isaacroldan isaacroldan requested a review from a team as a code owner April 22, 2025 12:05
gonzaloriestra
gonzaloriestra approved these changes Apr 24, 2025
View reviewed changes

@gonzaloriestra gonzaloriestra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Checked that it works as expected 👌

What about adding a test?

@isaacroldan Graphite App

isaacroldan commented Apr 24, 2025

Copy link
Copy Markdown
Contributor Author

I've updated an existing test to check that we don't pass the cached config name. Since link is mocked, what other test do you think I could add? or should I try not to mock link?

@isaacroldan isaacroldan added this pull request to the merge queue Apr 25, 2025
Merged via the queue into main with commit e28555c Apr 25, 2025
@isaacroldan isaacroldan deleted the 04-22-fix_behavior_of_--reset_not_creating_a_new_toml_file branch April 25, 2025 08:12
timothytenzin20 added a commit that referenced this pull request Apr 9, 2026
@timothytenzin20
Fix --reset flag regression: skip spurious TOML selection prompt befo…
727472f 
…re link flow

When running `shopify app dev --reset`, `getAppConfigurationContext()` was
called unconditionally before checking `forceRelink`. If the cached config
file didn't exist on disk, this triggered `selectConfigFile()`, prompting
the user to pick a TOML file that would be immediately discarded by `link()`.

Regression introduced by #6612 ("Remove legacy app schema", commit 1f8dcce)
which merged the forceRelink and !isLinked branches back together, undoing
the prior fix from #5676.

Fix: separate the two paths so forceRelink skips `getAppConfigurationContext()`
entirely and calls `link()` directly.

Fixes shop/issues-develop#22531
@timothytenzin20 timothytenzin20 mentioned this pull request Apr 9, 2026
Merged
timothytenzin20 added a commit that referenced this pull request Apr 13, 2026
@timothytenzin20
Fix --reset flag regression: skip spurious TOML selection prompt befo…
2b55007 
…re link flow

When running `shopify app dev --reset`, `getAppConfigurationContext()` was
called unconditionally before checking `forceRelink`. If the cached config
file didn't exist on disk, this triggered `selectConfigFile()`, prompting
the user to pick a TOML file that would be immediately discarded by `link()`.

Regression introduced by #6612 ("Remove legacy app schema", commit 1f8dcce)
which merged the forceRelink and !isLinked branches back together, undoing
the prior fix from #5676.

Fix: separate the two paths so forceRelink skips `getAppConfigurationContext()`
entirely and calls `link()` directly.

Fixes shop/issues-develop#22531
timothytenzin20 added a commit that referenced this pull request Apr 14, 2026
@timothytenzin20
Fix --reset flag regression: skip spurious TOML selection prompt befo…
df96a96 
…re link flow

When running `shopify app dev --reset`, `getAppConfigurationContext()` was
called unconditionally before checking `forceRelink`. If the cached config
file didn't exist on disk, this triggered `selectConfigFile()`, prompting
the user to pick a TOML file that would be immediately discarded by `link()`.

Regression introduced by #6612 ("Remove legacy app schema", commit 1f8dcce)
which merged the forceRelink and !isLinked branches back together, undoing
the prior fix from #5676.

Fix: separate the two paths so forceRelink skips `getAppConfigurationContext()`
entirely and calls `link()` directly.

Fixes shop/issues-develop#22531
gonzaloriestra pushed a commit that referenced this pull request Apr 15, 2026
@timothytenzin20
Fix --reset flag regression: skip spurious TOML selection prompt befo…
18221d6 
…re link flow

When running `shopify app dev --reset`, `getAppConfigurationContext()` was
called unconditionally before checking `forceRelink`. If the cached config
file didn't exist on disk, this triggered `selectConfigFile()`, prompting
the user to pick a TOML file that would be immediately discarded by `link()`.

Regression introduced by #6612 ("Remove legacy app schema", commit 1f8dcce)
which merged the forceRelink and !isLinked branches back together, undoing
the prior fix from #5676.

Fix: separate the two paths so forceRelink skips `getAppConfigurationContext()`
entirely and calls `link()` directly.

Fixes shop/issues-develop#22531
gonzaloriestra pushed a commit that referenced this pull request Apr 15, 2026
@timothytenzin20
Fix --reset flag regression: skip spurious TOML selection prompt befo…
bb884b1 
…re link flow

When running `shopify app dev --reset`, `getAppConfigurationContext()` was
called unconditionally before checking `forceRelink`. If the cached config
file didn't exist on disk, this triggered `selectConfigFile()`, prompting
the user to pick a TOML file that would be immediately discarded by `link()`.

Regression introduced by #6612 ("Remove legacy app schema", commit 1f8dcce)
which merged the forceRelink and !isLinked branches back together, undoing
the prior fix from #5676.

Fix: separate the two paths so forceRelink skips `getAppConfigurationContext()`
entirely and calls `link()` directly.

Fixes shop/issues-develop#22531
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gonzaloriestra gonzaloriestra gonzaloriestra approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: app deploy --reset no longer resets toml file configuration

2 participants

@isaacroldan @gonzaloriestra