Don't link on deploy when api key is provided #4261
Conversation
|
Thanks for your contribution! Depending on what you are working on, you may want to request a review from a Shopify team:
|
Coverage report
Show files with reduced coverage 🔻
Test suite run success1802 tests passing in 822 suites. Report generated by 🧪jest coverage report action from 3b1ec3d |
This comment has been minimized.
This comment has been minimized.
| @@ -430,7 +430,7 @@ export async function ensureDeployContext(options: DeployContextOptions): Promis | |||
| const {reset, force, noRelease} = options | |||
| let developerPlatformClient = options.developerPlatformClient | |||
| // do the link here | |||
| const [remoteApp] = await fetchAppAndIdentifiers(options, developerPlatformClient, true, true) | |||
| const [remoteApp] = await fetchAppAndIdentifiers(options, developerPlatformClient, true, !options.apiKey) | |||
There was a problem hiding this comment.
Positional arguments here makes it hard to see, so here's a link to the API we're calling to demonstrate that the enableLinkingPrompt is what changed:
cli/packages/app/src/cli/services/context.ts
Line 645 in 18fae61
There was a problem hiding this comment.
Yeah, multiple booleans are pretty bad. We should revisit the public API of fetchAppAndIdentifiers. Usually we'd use an options object in this sort of case. Thanks for the note!
There was a problem hiding this comment.
The main goal with my PR is ensuring that, post-deploy, your TOML file is correctly configured and linked to the remote app. This requires a manual prompt usually, which is what broke the functions CI tests.
When providing the api key, it still feels like we should be able to link and update the TOML correctly. The prompting step is unnecessary because the user has provided the identifier. But the rest of the linking should be doable, right?
Doing the above would likely blow up the scope of this PR, so I'm ok shipping this to unblock functions, but we should at least queue up an issue to tackle the "proper" refactor later.
Approval conditional on getting that issue created (assuming folks agree) and tophatting working, but otherwise 🚢
| @@ -430,7 +430,7 @@ export async function ensureDeployContext(options: DeployContextOptions): Promis | |||
| const {reset, force, noRelease} = options | |||
| let developerPlatformClient = options.developerPlatformClient | |||
| // do the link here | |||
| const [remoteApp] = await fetchAppAndIdentifiers(options, developerPlatformClient, true, true) | |||
| const [remoteApp] = await fetchAppAndIdentifiers(options, developerPlatformClient, true, !options.apiKey) | |||
There was a problem hiding this comment.
Is this logic something we only want to apply for deploy? For example, release also works the same way. fetchAppAndIdentifiers has access to options so in theory we could do this logic inside of fetchAppAndIdentifiers if we wanted to apply this business logic to anyone who is trying to use this method.
No strong opinions...just asking...
|
Tophatted against this bug, found no regressions. |
|
Tophatted and it worked 🎩 ✅ |
Sounds good- just created an issue on the same repository the original bug came from: https://github.com/Shopify/develop-app-inner-loop/issues/2001 |
github-merge-queue
bot
removed this pull request from the merge queue due to Branch Protection failures
WHY are these changes introduced?
Fixes failing buildkites: https://buildkite.com/shopify/functions-platform-tests/builds/7325#01910f81-3ad3-4460-8d40-c577a22e4f1f
In non-interactive environments, CLI users pass in an
api-keyorclient-idflag to indicate which app they want to use. When this is the case, logic in the CLI needs to not try and prompt the user to select an app, or the process will fail.This code path was always setting
enableLinkingPromptto true, when really it should have been dependent on whether or not theapi-keyflag was passed in. This resulted in the CLI always trying to link the app with the remote app.WHAT is this pull request doing?
I set the value of
enableLinkingPromptto equal!options.apiKey, similar to how thedevcodepath handles this scenario:cli/packages/app/src/cli/services/context.ts
Line 167 in c7b83d8
How to test your changes?
~/Library/Preferences/shopify-cli-app-nodejs/config.jsonbased on my own machine.| catat the end of the command)Checklist