Repository navigation
Conversation
github-actions
Bot
added
the
no-changelog
Every place that reads SHOPIFY_APP_AUTOMATION_TOKEN or SHOPIFY_CLI_PARTNERS_TOKEN now goes through a single module, so later changes to token selection land in one place. Authentication uses getAutomationToken; analytics uses peekAutomationToken, which will stay free of validation so reporting never fails a command. Precedence is unchanged: the app variable wins whenever it is set, and an empty value means no token. getAppAutomationToken keeps its current behavior for external callers. Co-authored-by: Eddie Gao <eddie.gao@shopify.com>
47cd7a5 to
4e2f86e
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationspackages/cli-kit/dist/private/node/session/automation-token.d.ts/**
* The environment variable an automation token was read from.
*/
type AutomationTokenSource = 'app' | 'partners';
interface AutomationToken {
value: string;
source: AutomationTokenSource;
}
/**
* Reads the automation token from the environment without validating it, so it never throws.
*
* Telemetry reads the token through this function because reporting must not fail a command.
* Authentication reads it through `getAutomationToken`.
*
* SHOPIFY_APP_AUTOMATION_TOKEN takes precedence over the deprecated SHOPIFY_CLI_PARTNERS_TOKEN whenever it is
* set, even to an empty value. An empty value means no token.
*
* @param env - Environment variables to read the token from.
* @returns The automation token and the variable it came from, or undefined when there is none.
*/
export declare function peekAutomationToken(env?: NodeJS.ProcessEnv): AutomationToken | undefined;
/**
* Selects the automation token to authenticate with.
*
* @param env - Environment variables to read the token from.
* @returns The automation token and the variable it came from, or undefined when there is none.
*/
export declare function getAutomationToken(env?: NodeJS.ProcessEnv): AutomationToken | undefined;
export {};
Existing type declarationsWe found no diffs with existing type declarations |
| */ | ||
| export function getAutomationToken(env: NodeJS.ProcessEnv = process.env): AutomationToken | undefined { | ||
| return peekAutomationToken(env) | ||
| } |
There was a problem hiding this comment.
I don't understand, why the two functions? this one doesn't do anything differently 🤔
Why in some places you use one or the other?
There was a problem hiding this comment.
Its scaffolding for the next PRs... getting ahead of itself slightly though lol
There was a problem hiding this comment.
True- we only need this split in #8731 once we make one of them throw. Most likely closing this as it isn't required here at all
| export function peekAutomationToken(env: NodeJS.ProcessEnv = process.env): AutomationToken | undefined { | ||
| const appToken = env[environmentVariables.appAutomationToken] | ||
| if (appToken !== undefined) return appToken === '' ? undefined : {value: appToken, source: 'app'} | ||
|
|
||
| const partnersToken = env[environmentVariables.partnersToken] | ||
| return partnersToken ? {value: partnersToken, source: 'partners'} : undefined | ||
| } |
There was a problem hiding this comment.
What is the benefit in this interface design? Do we ever actually care about the source returned? I don't think so. I think if anything you should be able to specify which exact token you want back (since some flows require certain vars, mainly partners) and then app tokens can be aliased/rolled out together, where automation env var takes precedence, but the old env var will always be able to be used too. It would make ripping the partner token code out easier once we're able to as well.
4 participants
Part of shop/issues-develop#24004
TL;DR: Moves every read of the automation token variables behind one selector, so the next two PRs change token selection in one place. No behavior change.
SHOPIFY_ORGANIZATION_AUTOMATION_TOKENand the selection rules (draft until the name is confirmed)WHY are these changes introduced?
SHOPIFY_APP_AUTOMATION_TOKENandSHOPIFY_CLI_PARTNERS_TOKENare read in five places acrosspublic/node/session.tsandprivate/node/session.ts. #8731 adds a third variable plus conflict and empty-value rules. Without a single selector, each rule would have to be copied into all five places.WHAT is this pull request doing?
getAutomationToken(new, inprivate/node/session/automation-token.ts) is what authentication calls. It returns{value, source}, and Add SHOPIFY_ORGANIZATION_AUTOMATION_TOKEN #8731 adds validation to it.peekAutomationTokenis what analytics calls (getLastSeenUserIdAfterAuth,getLastSeenAuthMethod). It never validates, so once Add SHOPIFY_ORGANIZATION_AUTOMATION_TOKEN #8731 adds errors, a misconfigured environment still can't drop a command's analytics event.getAppAutomationToken()is untouched. It's a publiccli-kitexport, so external callers see no change.One difference, analytics only: when
SHOPIFY_APP_AUTOMATION_TOKENis set but empty and a theme token is present, the analytics user ID now comes from the theme token instead of being skipped.How to manually test your changes?
With an App Automation Token and no cached login (
shopify auth logout), both of these behave exactly as onmain:Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add