feat: ci awareness to disable terminal prompting within commands #449

erunion · 2022-02-24T06:28:49Z

🚥 Fix RM-3621

🧰 Changes

With our new workflow in #437 there's a new issue that will likely crop up for some folks is that if you don't supply all of the required arguments for a command in a CI environment, our enquirer setup will prompt the terminal for input -- something that will never be answered in a CI environment.

For this we're introducing some new awareness to our commands that ask the user for input to not do that if it's within a CI environment and instead throw an error telling them that they're missing arguments.

I'm also making some other adjustments to a couple commands to clean up their argument structure:

versions:create
- --main and --beta are now booleans
- --isPublic has been renamed to just --public and it's also been made into a boolean.
- Make sure usage in README.md is updated
versions:update
- --main and --beta are now booleans
- --isPublic has been renamed to just --public and it's also been made into a boolean.
- Make sure usage in README.md is updated

🧬 QA & Testing

If with all the changes I made tests are still passing, 👍

You can also test this behavior out locally by prefixing GITHUB_ACTIONS=true to a ./bin/rdme call:

$ GITHUB_ACTIONS=true ./bin/rdme versions:create --version=1.0.0

We've detected you're running within a GitHub Actions environment, please supply the following required 
arguments for your command instead of relying on our terminal prompt system: fork

erunion · 2022-02-24T06:31:23Z

__tests__/cmds/versions.test.js

@@ -37,6 +37,8 @@ jest.mock('../../src/lib/prompts');
 describe('rdme versions*', () => {
  beforeAll(() => nock.disableNetConnect());

+  beforeEach(() => jest.resetModules());


Because we're mocking out process.env.CIRCLECI to test the ci-info package if we don't reset modules here then that package will get loaded within our src/lib/enquirer.js wrapper without the env variable, and cached -- so when the test that does need it, and does mock that environment variable out, the cached module doesn't see it.

I feel like it might make more sense to do this in a global beforeEach call at some point

erunion · 2022-02-24T06:32:38Z

__tests__/cmds/versions.test.js

      mockRequest.done();
    });

+    it.todo('should prompt a version list if no `--fork` argument is supplied');


Not having something like promptHandler.createVersionPrompt.mockResolvedValue to mock Enquirer responses with this new setup is annoying but as it is I'm comfortable with the tradeoff we're making with this work and not having to have tests for Enquirer being able to prompt us for questions.

_{_{though i'm sure i'l regret this a year from now}}

lmaoo yeah I like that we can mock and reuse responses with that promptHandler, but we have a lot of different patterns in that file that are pretty confusing so I think we should revisit that at some point

src/cmds/versions/create.js

erunion · 2022-02-24T06:34:29Z

src/cmds/versions/create.js

+      {
+        type: 'select',
+        name: 'fork',
+        message: 'Which version would you like to fork from?',


Having these questions duped in some commands sucks but I'm comfortable with the tradeoff of having duped questions in two commands than having to maintain a question generation library.

erunion · 2022-02-24T06:35:22Z

src/lib/enquirer.js

+          `We've detected you're running within a ${
+            ci.name
+          } environment, please supply the following required arguments for your command instead of relying on our terminal prompt system: ${chalk.yellow(
+            missingArgs.join(',')


I do not like this sentence I've written; please suggest something better.

erunion · 2022-02-24T06:53:37Z

__tests__/cmds/versions.test.js

+
+    describe('ci environments', () => {
+      beforeEach(() => {
+        process.env.CIRCLECI = true;


Purposefully testing it with CIRCLECI here instead of GITHUB_ACTIONS because I know that GITHUB_ACTIONS will always be present in our CI and I don't want to delete that from the workflow runner.

good thinking!

erunion · 2022-02-24T07:08:58Z

__tests__/cmds/versions.test.js

-        .basicAuth({ user: key })
-        .reply(200, [{ version }, { version }])
-        .post('/api/v1/version')
+        .post('/api/v1/version', {


We weren't fully testing out the expected API payload. Wish there were a way for us to configure Nock to require this...

kanadgupta

I have one general question about the default export from the enquirer handler (and a few minor comments/suggestions). Big fan of this pattern overall 👏🏽

kanadgupta · 2022-02-24T15:02:03Z

__tests__/cmds/versions.test.js

@@ -37,6 +37,8 @@ jest.mock('../../src/lib/prompts');
 describe('rdme versions*', () => {
  beforeAll(() => nock.disableNetConnect());

+  beforeEach(() => jest.resetModules());


I feel like it might make more sense to do this in a global beforeEach call at some point

kanadgupta · 2022-02-24T15:05:20Z

__tests__/cmds/versions.test.js

      mockRequest.done();
    });

+    it.todo('should prompt a version list if no `--fork` argument is supplied');


lmaoo yeah I like that we can mock and reuse responses with that promptHandler, but we have a lot of different patterns in that file that are pretty confusing so I think we should revisit that at some point

kanadgupta · 2022-02-24T15:05:33Z

__tests__/cmds/versions.test.js

+
+    describe('ci environments', () => {
+      beforeEach(() => {
+        process.env.CIRCLECI = true;


good thinking!

kanadgupta · 2022-02-24T15:06:23Z

__tests__/cmds/versions.test.js

+          .reply(200, [{ version }, { version }]);
+
+        await expect(createVersion.run({ key, version })).rejects.toThrow(
+          /We've detected you're running within a (.*) environment, please supply the following required arguments for your command instead of relying on our terminal prompt system: fork/


Any reason why we can't just test for an exact match here?

Because even though I'm setting CIRCLECI to the environment, GITHUB_ACTIONS is also present and ci-info picks that up first so this message says "We've detected you're running within a Github Actions environment" in CI and "within a CircleCI environment" locally.

src/cmds/versions/create.js

src/lib/enquirer.js

kanadgupta · 2022-02-24T15:43:14Z

src/lib/enquirer.js

+  return enquirer.prompt(questions).catch(err => {
+    // If the Enquirer promise was rejected without an error then the user killed an open prompt so
+    // we should abide by their wishes and bail out.
+    if (!err) process.exit(0);


I feel like we could return a 1 here since it's technically not a success, right? Not sure what the best practices are here 🤔

Exit code 1 generally means that that app executed with errors but since in this case I'm killing the prompt myself it's not really an error. Jest also returns the same 0 exit code when you exit out of an open --watch.

Exiting Jest before it's able to load any tests:

Exiting an open --watch process:

kanadgupta · 2022-02-24T15:45:29Z

src/lib/enquirer.js

+      // Purposefully loading `ci-info` here so we can adequately test it.
+      const ci = require('ci-info'); // eslint-disable-line global-require
+      if (!ci.isCI) {
+        // If we aren't in a CI environment then we shouldn't look for unanswered questions.
+        return prompt(list);
+      }


This is really clean 🤩

erunion · 2022-02-28T04:12:16Z

src/cmds/versions/create.js

+      {
+        type: 'confirm',
+        name: 'main',
+        message: 'Would you like to make this version the main version for this project?',


@kanadgupta Thoughts on slightly reworking this? The double "version" in there reads off.

Suggested change

message: 'Would you like to make this version the main version for this project?',

message: 'Would you like to make this the main version for this project?',

Agreed! Nice find

erunion · 2022-02-28T17:30:11Z

After talking through this this morning I'm deciding to close this work out. We have a lot of problems with our existing prompt system (difficult to work with, difficult to test, not every question is mapped to an argument) that while this work aimed to solve it solved it in a backwards way.

Instead of rejecting CI execution of these commands that likely will never be executed from a CI environment anyways, we should instead focus on cleaning up these individual commands instead.

feat: overhauling the version creation command to be CI aware

b35ed9b

erunion changed the title ~~feat: ci awareness within commands to disable terminal prompting~~ feat: ci awareness to disable terminal prompting within commands Feb 24, 2022

erunion changed the base branch from main to feat/drop-node12 February 24, 2022 06:29

erunion commented Feb 24, 2022

View reviewed changes

src/cmds/versions/create.js Show resolved Hide resolved

erunion commented Feb 24, 2022

View reviewed changes

erunion added 2 commits February 23, 2022 22:42

fix: fixing some weird logic issues with the version creation command

08828b5

fix: loosening up the ci env message assertion

f16ae27

erunion commented Feb 24, 2022

View reviewed changes

erunion added the enhancement New feature or request label Feb 24, 2022

erunion added this to the v7 milestone Feb 24, 2022

erunion commented Feb 24, 2022

View reviewed changes

kanadgupta requested changes Feb 24, 2022

View reviewed changes

erunion added 2 commits February 27, 2022 19:28

Merge branch 'feat/drop-node12' into feat/ci-awareness

3b87c36

fix: addressing pr feedback

f4e7834

erunion commented Feb 28, 2022

View reviewed changes

erunion closed this Feb 28, 2022

erunion deleted the feat/ci-awareness branch February 28, 2022 17:30

kanadgupta mentioned this pull request Aug 19, 2022

refactor: migrate from enquirer to prompts #570

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ci awareness to disable terminal prompting within commands #449

feat: ci awareness to disable terminal prompting within commands #449

erunion commented Feb 24, 2022 •

edited by kanadgupta

Loading

erunion Feb 24, 2022

kanadgupta Feb 24, 2022

erunion Feb 24, 2022 •

edited

Loading

kanadgupta Feb 24, 2022

erunion Feb 24, 2022

erunion Feb 24, 2022

erunion Feb 24, 2022

kanadgupta Feb 24, 2022

erunion Feb 24, 2022

kanadgupta left a comment

kanadgupta Feb 24, 2022

kanadgupta Feb 24, 2022

kanadgupta Feb 24, 2022

kanadgupta Feb 24, 2022

erunion Feb 24, 2022

kanadgupta Feb 24, 2022

erunion Feb 28, 2022

kanadgupta Feb 24, 2022

erunion Feb 28, 2022 •

edited

Loading

kanadgupta Feb 28, 2022

erunion commented Feb 28, 2022

	message: 'Would you like to make this version the main version for this project?',
	message: 'Would you like to make this the main version for this project?',

feat: ci awareness to disable terminal prompting within commands #449

feat: ci awareness to disable terminal prompting within commands #449

Conversation

erunion commented Feb 24, 2022 • edited by kanadgupta Loading

🧰 Changes

🧬 QA & Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erunion Feb 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanadgupta 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

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

erunion Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erunion commented Feb 28, 2022

erunion commented Feb 24, 2022 •

edited by kanadgupta

Loading

erunion Feb 24, 2022 •

edited

Loading

erunion Feb 28, 2022 •

edited

Loading