non-TTY required variables checks #852

JacobMGEvans · 2022-04-26T16:48:27Z

Added a check in non-TTY environments for account_id, CLOUDFLARE_ACCOUNT_ID and CLOUDFLARE_API_TOKEN. If account_id exists in wrangler.toml
then CLOUDFLARE_ACCOUNT_ID is not needed in non-TTY scope. The CLOUDFLARE_API_TOKEN is necessary in non-TTY scope and will always error if missing.

resolves #827

changeset-bot · 2022-04-26T16:48:30Z

🦋 Changeset detected

Latest commit: 40a5f50

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
wrangler	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2022-04-26T16:50:04Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2271600192/npm-package-wrangler-852

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/852/npm-package-wrangler-852

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/2271600192/npm-package-wrangler-852 dev path/to/script.js

threepointone

There's a better way to do this, which is to only throw the error when request. (So, inside getAccountId and getAPIToken. I'm on phone so I can't remember function names exactly). This would also remove any checks for test environments and such.

packages/wrangler/src/utils/ci-check.ts

JacobMGEvans · 2022-04-26T22:36:08Z

packages/wrangler/src/__tests__/sentry.test.ts

@@ -21,10 +22,13 @@ describe("Error Reporting", () => {
    jest.mock("@sentry/node");
    jest.spyOn(Sentry, "captureException");
    process.stdout.isTTY = true;
+    jest.mock("ci-info");
+    (ci.isCI as jest.Mocked<boolean>) = false;


ciCheck function doesn't make sense to modify to handle this use case, directly mocking ci-info
Alternatively we can make a simple wrapper function that is just returns a boolean from, ci.isCI

JacobMGEvans · 2022-04-26T22:37:21Z

packages/wrangler/src/__tests__/sentry.test.ts


-    expect(error_tracking_opt).toBe(false);
-    expect(error_tracking_opt_date).toBeTruthy();
+  it("should not prompt in CI/CD environment", async () => {


Added additional test that handles the change in reporting.ts that checks if its TTY or CI and does nothing now.

Cc: @petebacondarwin

JacobMGEvans · 2022-04-26T22:38:21Z

packages/wrangler/src/__tests__/sentry.test.ts

+    expect(
+      fs.existsSync(path.join(os.homedir(), reportingTOMLPath, "utf-8"))
+    ).toBe(false);


Change to reporting does nothing if in a TTY or CI environment, test reflects that by confirming no Reporting TOML is created.

packages/wrangler/src/utils/ci-check.ts

packages/wrangler/src/__tests__/sentry.test.ts

threepointone

it's looking good, marking as blocking so I can test it myself before we land it. thank you @JacobMGEvans!

petebacondarwin

In this new ciCheck() we are kind of reproducing work already done in user.tsx.

getAPIToken() and getAccountId() can already handle non-interactive cases, so I think we should just update those to throw errors rather than returning undefined if in non-TTY mode.

For example:

export function getAPIToken(): string | undefined {
  if (LocalState.apiToken) {
    logger.warn(
      "It looks like you have used Wrangler 1's `config` command to login with an API token.\n" +
        "This is no longer supported in the current version of Wrangler.\n" +
        "If you wish to authenticate via an API token then please set the `CLOUDFLARE_API_TOKEN` environment variable."
    );
  }
  if (process.stdin.isTTY) {
    return LocalState.accessToken?.value;
  } else {
    throw new Error(...);
  }
}

and

export async function getAccountId(
  isInteractive = true
): Promise<string | undefined> {
  const apiToken = getAPIToken();
  if (!apiToken) return;

  const accountIdFromEnv = getCloudflareAccountIdFromEnv();
  if (accountIdFromEnv) {
    return accountIdFromEnv;
  } else if (!isInteractive) {
    throw new Error(...);
  }
  ...

packages/wrangler/src/__tests__/ci.test.ts

packages/wrangler/src/ci-check.ts

JacobMGEvans · 2022-04-28T16:42:34Z

In this new ciCheck() we are kind of reproducing work already done in user.tsx.

getAPIToken() and getAccountId() can already handle non-interactive cases, so I think we should just update those to throw errors rather than returning undefined if in non-TTY mode.

For example: See quoted message

I don't see the reproducing of work, I can see the value of co-locating the checks, control logic, within the getAccountId and getAPIToken functions rather than having a function that handles checks for both functions and their error handling. My only concern would be mocking the isTTY check in tests throughout except for ci.test?

caass · 2022-04-28T19:11:06Z

I'm confused -- the issue we're trying to close with this PR is that we should enforce the use of an api token via environment variable when in CI, right? So then why are we checking process.stdout.isTTY? That can be false for a variety of reasons, e.g. we're running tests, a user is piping output to another command, etc. I feel like if the issue is one of CI, it makes sense to use something like is-ci (or at the very least, checking process.env.CI)

packages/wrangler/src/user.tsx

threepointone

This PR is missing checks for CLOUDFLARE_ACCOUNT_ID the way it's doing it for API_TOKEN. Did you plan on doing that in a later PR?

packages/wrangler/src/__tests__/publish.test.ts

threepointone · 2022-05-03T21:38:26Z

packages/wrangler/src/__tests__/publish.test.ts

+          .toMatchInlineSnapshot(`
+                [Error: More than one account available but unable to select one in non-interactive mode.
+                Please set the appropriate \`account_id\` in your \`wrangler.toml\` file.
+                Available accounts are ("<name>" - "<id>"):


I think the message shouldn't be to set the account_id in the toml file, but to prefer setting the CLOUDFLARE_ACCOUNT_ID env var. I also think you can suggest running wrangler whoami to get the account id value.

Perhaps suggest both options?

packages/wrangler/src/user.tsx

packages/wrangler/src/__tests__/publish.test.ts

Added a check in non-TTY environments for `account_id`, `CLOUDFLARE_ACCOUNT_ID` and `CLOUDFLARE_API_TOKEN`. If `account_id` exists in `wrangler.toml` then `CLOUDFLARE_ACCOUNT_ID` is not needed in non-TTY scope. The `CLOUDFLARE_API_TOKEN` is necessary in non-TTY scope and will always error if missing. resolves #827

threepointone

it's getting close! one last note regarding a user without any accounts

threepointone · 2022-05-04T18:16:15Z

packages/wrangler/src/__tests__/secret.test.ts

@@ -169,10 +171,14 @@ describe("wrangler secret", () => {
        mockAccountId({ accountId: null });

        it("should error if a user has no account", async () => {
+          mockGetMemberships({


I don't think this will ever happen. If a user has no account, then the api will throw a 10000 error.

You can reproduce this locally with a garbage invalid token.

This was a preexisting test, I just updated to using a new mock for consistency.

The exisitng test had the right error message tho, this isn't the right mock. It will never return a list with zero memberships.

Oh hmm I see. Aight never mind then.

threepointone · 2022-05-04T18:17:20Z

packages/wrangler/src/__tests__/secret.test.ts

          await expect(
            runWrangler("secret put the-key --name script-name")
          ).rejects.toThrowErrorMatchingInlineSnapshot(
-            `"No account id found, quitting..."`
+            `"Failed to automatically retrieve account IDs for the logged in user. In a non-interactive environment, it is mandatory to specify an account ID, either by assigning its value to CLOUDFLARE_ACCOUNT_ID, or as \`account_id\` in your \`wrangler.toml\` file."`


which will make this error be "No account id found, quitting..."

The test is occuring in a "non-TTY" block

packages/wrangler/src/__tests__/publish.test.ts

petebacondarwin · 2022-05-04T18:23:46Z

packages/wrangler/src/__tests__/publish.test.ts

+          .toMatchInlineSnapshot(`
+                [Error: More than one account available but unable to select one in non-interactive mode.
+                Please set the appropriate \`account_id\` in your \`wrangler.toml\` file.
+                Available accounts are ("<name>" - "<id>"):


Perhaps suggest both options?

Added a check in non-TTY environments for `account_id`, `CLOUDFLARE_ACCOUNT_ID` and `CLOUDFLARE_API_TOKEN`. If `account_id` exists in `wrangler.toml` then `CLOUDFLARE_ACCOUNT_ID` is not needed in non-TTY scope. The `CLOUDFLARE_API_TOKEN` is necessary in non-TTY scope and will always error if missing. resolves #827

threepointone

Let's land it and try this out.

JacobMGEvans added the polish Small improvements to the experience label Apr 26, 2022

JacobMGEvans requested review from threepointone and petebacondarwin as code owners April 26, 2022 16:48

JacobMGEvans force-pushed the ci-variable-checks branch from 06f5e6d to 607b433 Compare April 26, 2022 16:49

JacobMGEvans force-pushed the ci-variable-checks branch 2 times, most recently from 927fa20 to f3f6e6c Compare April 26, 2022 19:00

threepointone requested changes Apr 26, 2022

View reviewed changes

JacobMGEvans force-pushed the ci-variable-checks branch 3 times, most recently from 1ab1306 to afa1b39 Compare April 26, 2022 19:51

JacobMGEvans changed the title ~~CI/CD necessary variable checks~~ CI/CD required variables checks Apr 26, 2022

JacobMGEvans force-pushed the ci-variable-checks branch from afa1b39 to 27c024a Compare April 26, 2022 22:32

JacobMGEvans commented Apr 26, 2022

View reviewed changes

packages/wrangler/src/utils/ci-check.ts Outdated Show resolved Hide resolved

JacobMGEvans commented Apr 26, 2022

View reviewed changes

JacobMGEvans requested a review from threepointone April 26, 2022 22:39

threepointone reviewed Apr 27, 2022

View reviewed changes

packages/wrangler/src/utils/ci-check.ts Outdated Show resolved Hide resolved

packages/wrangler/src/utils/ci-check.ts Outdated Show resolved Hide resolved

JacobMGEvans force-pushed the ci-variable-checks branch from 27c024a to 1adc237 Compare April 27, 2022 15:05

JacobMGEvans commented Apr 27, 2022

View reviewed changes

packages/wrangler/src/__tests__/sentry.test.ts Show resolved Hide resolved

JacobMGEvans force-pushed the ci-variable-checks branch from 1adc237 to 2413b3c Compare April 27, 2022 15:17

JacobMGEvans requested a review from threepointone April 27, 2022 15:17

JacobMGEvans force-pushed the ci-variable-checks branch 2 times, most recently from 87cb0d2 to 7e63bd5 Compare April 27, 2022 15:20

threepointone requested changes Apr 27, 2022

View reviewed changes

petebacondarwin requested changes Apr 28, 2022

View reviewed changes

packages/wrangler/src/__tests__/ci.test.ts Outdated Show resolved Hide resolved

packages/wrangler/src/ci-check.ts Outdated Show resolved Hide resolved

packages/wrangler/src/ci-check.ts Outdated Show resolved Hide resolved

JacobMGEvans force-pushed the ci-variable-checks branch 2 times, most recently from a3c07af to a3757e9 Compare May 2, 2022 19:33

JacobMGEvans changed the title ~~CI/CD required variables checks~~ non-TTY required variables checks May 2, 2022

JacobMGEvans force-pushed the ci-variable-checks branch from a3757e9 to 45b4735 Compare May 2, 2022 21:50

JacobMGEvans commented May 2, 2022

View reviewed changes

packages/wrangler/src/user.tsx Show resolved Hide resolved

JacobMGEvans force-pushed the ci-variable-checks branch from 45b4735 to e50fa70 Compare May 2, 2022 21:59

petebacondarwin force-pushed the ci-variable-checks branch from e50fa70 to cd17369 Compare May 3, 2022 14:26

JacobMGEvans marked this pull request as ready for review May 3, 2022 15:05

JacobMGEvans requested review from petebacondarwin and threepointone May 3, 2022 15:05

threepointone requested changes May 3, 2022

View reviewed changes

packages/wrangler/src/__tests__/publish.test.ts Show resolved Hide resolved

packages/wrangler/src/__tests__/publish.test.ts Outdated Show resolved Hide resolved

threepointone reviewed May 3, 2022

View reviewed changes

JacobMGEvans force-pushed the ci-variable-checks branch 3 times, most recently from 6c3a0e4 to ce9db80 Compare May 4, 2022 16:21

threepointone reviewed May 4, 2022

View reviewed changes

packages/wrangler/src/user.tsx Outdated Show resolved Hide resolved

packages/wrangler/src/user.tsx Outdated Show resolved Hide resolved

packages/wrangler/src/__tests__/publish.test.ts Show resolved Hide resolved

JacobMGEvans force-pushed the ci-variable-checks branch from ce9db80 to f1c3343 Compare May 4, 2022 17:50

JacobMGEvans force-pushed the ci-variable-checks branch 4 times, most recently from 16b92d5 to dca0292 Compare May 4, 2022 18:02

threepointone reviewed May 4, 2022

View reviewed changes

petebacondarwin approved these changes May 4, 2022

View reviewed changes

JacobMGEvans force-pushed the ci-variable-checks branch from dca0292 to 40a5f50 Compare May 4, 2022 18:27

threepointone approved these changes May 4, 2022

View reviewed changes

JacobMGEvans merged commit 6283ad5 into main May 4, 2022

JacobMGEvans deleted the ci-variable-checks branch May 4, 2022 18:39

github-actions bot mentioned this pull request May 4, 2022

Version Packages #889

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

non-TTY required variables checks #852

non-TTY required variables checks #852

JacobMGEvans commented Apr 26, 2022 •

edited

Loading

changeset-bot bot commented Apr 26, 2022 •

edited

Loading

github-actions bot commented Apr 26, 2022 •

edited

Loading

threepointone left a comment

JacobMGEvans Apr 26, 2022

JacobMGEvans Apr 26, 2022

JacobMGEvans Apr 26, 2022

threepointone left a comment

petebacondarwin left a comment

JacobMGEvans commented Apr 28, 2022 •

edited

Loading

caass commented Apr 28, 2022 •

edited

Loading

threepointone left a comment

threepointone May 3, 2022

petebacondarwin May 4, 2022

threepointone left a comment

threepointone May 4, 2022

threepointone May 4, 2022

JacobMGEvans May 4, 2022

threepointone May 4, 2022 •

edited

Loading

threepointone May 4, 2022

threepointone May 4, 2022

JacobMGEvans May 4, 2022

petebacondarwin May 4, 2022

threepointone left a comment

non-TTY required variables checks #852

non-TTY required variables checks #852

Conversation

JacobMGEvans commented Apr 26, 2022 • edited Loading

changeset-bot bot commented Apr 26, 2022 • edited Loading

🦋 Changeset detected

github-actions bot commented Apr 26, 2022 • edited Loading

threepointone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

threepointone left a comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

JacobMGEvans commented Apr 28, 2022 • edited Loading

caass commented Apr 28, 2022 • edited Loading

threepointone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

threepointone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

threepointone May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

threepointone left a comment

Choose a reason for hiding this comment

JacobMGEvans commented Apr 26, 2022 •

edited

Loading

changeset-bot bot commented Apr 26, 2022 •

edited

Loading

github-actions bot commented Apr 26, 2022 •

edited

Loading

JacobMGEvans commented Apr 28, 2022 •

edited

Loading

caass commented Apr 28, 2022 •

edited

Loading

threepointone May 4, 2022 •

edited

Loading