Repository: Redocly/redocly-cli
Pull request #1137 (2023-06-21 11:40:54)

feat: update push command
## What/Why/How?

- Add new options `--organization` and `--destination`.
- Mark `maybeApiOrDestination` and `maybeDestination` as deprecated
- Rename `job-id` option and mark `batch-id` as deprecated

## Reference

Closes: https://github.com/Redocly/redocly-cli/issues/1032

## Testing

## Screenshots (optional)
<img width="678" alt="Знімок екрана 2023-06-26 о 17 18 49" src="https://github.com/Redocly/redocly-cli/assets/106662428/fa8788ef-b1f4-47dc-bb11-16b19a307d65">

## Check yourself

- [x] Code is linted
- [x] Tested with redoc/reference-docs/workflows
- [x] All new/updated code is covered with tests

## Security

- [x] Security impact of change has been considered
- [x] Code follows company security practices and guidelines


github-actions[bot]: | Command | Mean [s] | Min [s] | Max [s] | Relative |
|:---|---:|---:|---:|---:|
| `redocly lint packages/core/src/benchmark/benches/rebilly.yaml` | 1.702 ± 0.086 | 1.615 | 1.874 | 1.03 ± 0.06 |
| `redocly-next lint packages/core/src/benchmark/benches/rebilly.yaml` | 1.654 ± 0.055 | 1.583 | 1.742 | 1.00 |

<!-- thollander/actions-comment-pull-request "hyperfine-comparison" -->
github-actions[bot]: <!-- jest coverage report action for options with hash 3339ae53a38c44382c0fb4491bdb282d -->

## Coverage report 



| <div title="Status of coverage:&#10; 🟢 - ok&#10; 🟡 - slightly more than threshold&#10; 🔴 - under the threshold">St.<sup>:grey_question:</sup></div> | Category   | Percentage | Covered / Total |
| :----------------------------------------------------------------------------------------------------------------------------------------------------: | :--------- | :--------- | :-------------: |
|                                                                           🟡                                                                           | Statements | 73.35%     |    3528/4810    |
|                                                                           🟡                                                                           | Branches   | 65.05%     |    2014/3096    |
|                                                                           🟡                                                                           | Functions  | 65.7%      |     590/898     |
|                                                                           🟡                                                                           | Lines      | 73.4%      |    3284/4474    |


## Test suite run success
589 tests passing in 86 suites.

<p align="right">Report generated by <a href="https://github.com/ArtiomTr/jest-coverage-report-action">🧪jest coverage report action</a> from 101ccb5224efc4f519a8ac2c1c0c245293f033f4</p>

lornajane: The usage instruction `redocly push --help` isn't correct. It still lists the "maybe" parameters - these should be removed. There should be a single positional. I think the deprecated `--batch-id` should also be hidden (I will include it in the migration guides for anyone coming from an earlier version, which means it'll show up in the search results on the docs if people are looking for it).
lornajane: Please capture a followup issue against the Workflows project to update the push instructions - the old commands work, but the output warnings so updating the instructions will give a better user experience (and probably fewer support tickets).
tatomyr: The related changes in Workflows: https://github.com/Redocly/workflows/pull/2644

@lornajane FYI
IgorKarpiuk: > The usage instruction `redocly push --help` isn't correct. It still lists the "maybe" parameters - these should be removed. There should be a single positional. I think the deprecated `--batch-id` should also be hidden (I will include it in the migration guides for anyone coming from an earlier version, which means it'll show up in the search results on the docs if people are looking for it).

Updated usage instruction  for push command please check
lornajane: We are missing help text for the branch, organization, destination, and upsert parameters. Is it simplest for me to just add those quickly?
tatomyr: @lornajane please go ahead!

lornajane: (hang on, I'll just fix that build failure, sorry!)

Patch:
diff --git a/packages/cli/src/commands/push.ts b/packages/cli/src/commands/push.ts
index c4423edb..b2f3a029 100644
--- a/packages/cli/src/commands/push.ts
+++ b/packages/cli/src/commands/push.ts
@@ -2,7 +2,7 @@ import * as fs from 'fs';
 import * as path from 'path';
 import fetch from 'node-fetch';
 import { performance } from 'perf_hooks';
-import { yellow, green, blue } from 'colorette';
+import { yellow, green, blue, red } from 'colorette';
 import { createHash } from 'crypto';
 import {
   bundle,
@@ -34,12 +34,13 @@ export type PushOptions = {
   destination?: string;
   branchName?: string;
   upsert?: boolean;
-  'batch-id'?: string;
+  'job-id'?: string;
   'batch-size'?: number;
   region?: Region;
   'skip-decorator'?: string[];
   public?: boolean;
   files?: string[];
+  organization?: string;
   config?: string;
 };
 
@@ -54,26 +55,28 @@ export async function handlePush(argv: PushOptions, config: Config): Promise<voi
   const startedAt = performance.now();
   const { destination, branchName, upsert } = argv;
 
-  const batchId = argv['batch-id'];
+  const jobId = argv['job-id'];
   const batchSize = argv['batch-size'];
 
   if (destination && !DESTINATION_REGEX.test(destination)) {
     exitWithError(
       `Destination argument value is not valid, please use the right format: ${yellow(
-        '<@organization-id/api-name@api-version>'
+        '<api-name@api-version>'
       )}`
     );
   }
 
-  const { organizationId, name, version } = getDestinationProps(destination, config.organization);
+  const destinationProps = getDestinationProps(destination, config.organization);
+
+  const organizationId = argv.organization || destinationProps.organizationId;
+  const { name, version } = destinationProps;
 
   if (!organizationId) {
     return exitWithError(
-      `No organization provided, please use the right format: ${yellow(
-        '<@organization-id/api-name@api-version>'
-      )} or specify the 'organization' field in the config file.`
+      `No organization provided, please use --organization option or specify the 'organization' field in the config file.`
     );
   }
+
   const api = argv.api || (name && version && getApiRoot({ name, version, config }));
 
   if (name && version && !api) {
@@ -84,9 +87,16 @@ export async function handlePush(argv: PushOptions, config: Config): Promise<voi
     );
   }
 
-  if (batchId && !batchId.trim()) {
+  // Ensure that a destination for the api is provided.
+  if (!name && api) {
+    return exitWithError(
+      `No destination provided, please use --destination option to provide destination.`
+    );
+  }
+
+  if (jobId && !jobId.trim()) {
     exitWithError(
-      `The ${blue(`batch-id`)} option value is not valid, please avoid using an empty string.`
+      `The ${blue(`job-id`)} option value is not valid, please avoid using an empty string.`
     );
   }
 
@@ -169,7 +179,7 @@ export async function handlePush(argv: PushOptions, config: Config): Promise<voi
         branch: branchName,
         isUpsert: upsert,
         isPublic: argv['public'],
-        batchId: batchId,
+        batchId: jobId,
         batchSize: batchSize,
       });
     } catch (error) {
@@ -178,12 +188,13 @@ export async function handlePush(argv: PushOptions, config: Config): Promise<voi
       }
 
       if (error.message === 'API_VERSION_NOT_FOUND') {
-        exitWithError(`The definition version ${blue(name)}/${blue(
-          version
-        )} does not exist in organization ${blue(organizationId)}!\n${yellow(
-          'Suggestion:'
-        )} please use ${blue('-u')} or ${blue('--upsert')} to create definition.
-        `);
+        exitWithError(
+          `The definition version ${blue(
+            `${name}@${version}`
+          )} does not exist in organization ${blue(organizationId)}!\n${yellow(
+            'Suggestion:'
+          )} please use ${blue('-u')} or ${blue('--upsert')} to create definition.\n\n`
+        );
       }
 
       throw error;
@@ -328,34 +339,72 @@ export function getDestinationProps(
   }
 }
 
-type BarePushArgs = Omit<PushOptions, 'api' | 'destination' | 'branchName'> & {
-  maybeApiOrDestination?: string;
+type BarePushArgs = Omit<PushOptions, 'destination' | 'branchName'> & {
   maybeDestination?: string;
   maybeBranchName?: string;
   branch?: string;
+  destination?: string;
 };
 
 export const transformPush =
   (callback: typeof handlePush) =>
   (
-    { maybeApiOrDestination, maybeDestination, maybeBranchName, branch, ...rest }: BarePushArgs,
+    {
+      api: maybeApiOrDestination,
+      maybeDestination,
+      maybeBranchName,
+      branch,
+      'batch-id': batchId,
+      'job-id': jobId,
+      ...rest
+    }: BarePushArgs & { 'batch-id'?: string },
     config: Config
   ) => {
+    if (batchId) {
+      process.stderr.write(
+        yellow(
+          `The ${red('batch-id')} option is deprecated. Please use ${green('job-id')} instead.\n\n`
+        )
+      );
+    }
+
     if (maybeBranchName) {
       process.stderr.write(
         yellow(
-          'Deprecation warning: Do not use the third parameter as a branch name. Please use a separate --branch option instead.'
+          'Deprecation warning: Do not use the third parameter as a branch name. Please use a separate --branch option instead.\n\n'
         )
       );
     }
-    const api = maybeDestination ? maybeApiOrDestination : undefined;
-    const destination = maybeDestination || maybeApiOrDestination;
+
+    let apiFile, destination;
+    if (maybeDestination) {
+      process.stderr.write(
+        yellow(
+          'Deprecation warning: Do not use the second parameter as a destination. Please use a separate --destination and --organization instead.\n\n'
+        )
+      );
+      apiFile = maybeApiOrDestination;
+      destination = maybeDestination;
+    } else if (maybeApiOrDestination && DESTINATION_REGEX.test(maybeApiOrDestination)) {
+      process.stderr.write(
+        yellow(
+          'Deprecation warning: Do not use the first parameter as a destination. Please use a separate --destination and --organization options instead.\n\n'
+        )
+      );
+      destination = maybeApiOrDestination;
+    } else if (maybeApiOrDestination && !DESTINATION_REGEX.test(maybeApiOrDestination)) {
+      apiFile = maybeApiOrDestination;
+    }
+
+    destination = rest.destination || destination;
+
     return callback(
       {
         ...rest,
         destination,
-        api,
+        api: apiFile,
         branchName: branch ?? maybeBranchName,
+        'job-id': jobId || batchId,
       },
       config
     );
diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts
index 79821ed1..dc7b9986 100644
--- a/packages/cli/src/index.ts
+++ b/packages/cli/src/index.ts
@@ -126,22 +126,52 @@ yargs
       commandWrapper(handleJoin)(argv);
     }
   )
+
   .command(
-    'push [maybeApiOrDestination] [maybeDestination] [maybeBranchName]',
+    'push [api] [maybeDestination] [maybeBranchName]',
     'Push an API definition to the Redocly API registry.',
     (yargs) =>
       yargs
-        .positional('maybeApiOrDestination', { type: 'string' })
+        .usage('push [api]')
+        .positional('api', { type: 'string' })
         .positional('maybeDestination', { type: 'string' })
-        .positional('maybeBranchName', { type: 'string' })
+        .hide('maybeDestination')
+        .hide('maybeBranchName')
         .option({
-          branch: { type: 'string', alias: 'b' },
-          upsert: { type: 'boolean', alias: 'u' },
+          organization: {
+            description: 'Name of the organization to push to.',
+            type: 'string',
+            alias: 'o',
+          },
+          destination: {
+            description: 'API name and version in the format `name@version`.',
+            type: 'string',
+            alias: 'd',
+          },
+          branch: {
+            description: 'Branch name to push to.',
+            type: 'string',
+            alias: 'b',
+          },
+          upsert: {
+            description:
+              "Create the specified API version if it doesn't exist, update if it does exist.",
+            type: 'boolean',
+            alias: 'u',
+          },
           'batch-id': {
             description:
               'Specifies the ID of the CI job that the current push will be associated with.',
             type: 'string',
             requiresArg: true,
+            deprecated: true,
+            hidden: true,
+          },
+          'job-id': {
+            description:
+              'Specifies the ID of the CI job that the current push will be associated with.',
+            type: 'string',
+            requiresArg: true,
           },
           'batch-size': {
             description: 'Specifies the total number of CI jobs planned to be pushed.',
@@ -163,14 +193,12 @@ yargs
             array: true,
             type: 'string',
           },
-          config: {
-            description: 'Specify path to the config file.',
-            requiresArg: true,
-            type: 'string',
-          },
         })
+        .deprecateOption('batch-id', 'use --job-id')
+        .deprecateOption('maybeDestination')
+        .implies('job-id', 'batch-size')
         .implies('batch-id', 'batch-size')
-        .implies('batch-size', 'batch-id'),
+        .implies('batch-size', 'job-id'),
     (argv) => {
       process.env.REDOCLY_CLI_COMMAND = 'push';
       commandWrapper(transformPush(handlePush))(argv);


Test patch:
diff --git a/packages/cli/src/__tests__/commands/push.test.ts b/packages/cli/src/__tests__/commands/push.test.ts
index 6badbedc..a10c3b46 100644
--- a/packages/cli/src/__tests__/commands/push.test.ts
+++ b/packages/cli/src/__tests__/commands/push.test.ts
@@ -32,7 +32,7 @@ describe('push', () => {
         destination: '@org/my-api@1.0.0',
         branchName: 'test',
         public: true,
-        'batch-id': '123',
+        'job-id': '123',
         'batch-size': 2,
       },
       ConfigFixture as any
@@ -54,7 +54,7 @@ describe('push', () => {
     });
   });
 
-  it('fails if batchId value is an empty string', async () => {
+  it('fails if jobId value is an empty string', async () => {
     await handlePush(
       {
         upsert: true,
@@ -62,7 +62,7 @@ describe('push', () => {
         destination: '@org/my-api@1.0.0',
         branchName: 'test',
         public: true,
-        'batch-id': ' ',
+        'job-id': ' ',
         'batch-size': 2,
       },
       ConfigFixture as any
@@ -79,7 +79,7 @@ describe('push', () => {
         destination: '@org/my-api@1.0.0',
         branchName: 'test',
         public: true,
-        'batch-id': '123',
+        'job-id': '123',
         'batch-size': 1,
       },
       ConfigFixture as any
@@ -89,14 +89,9 @@ describe('push', () => {
   });
 
   it('push with --files', async () => {
-    // (loadConfigAndHandleErrors as jest.Mock).mockImplementation(({ files }) => {
-    //   return { ...ConfigFixture, files };
-    // });
-
     const mockConfig = { ...ConfigFixture, files: ['./resouces/1.md', './resouces/2.md'] } as any;
 
-    //@ts-ignore
-    fs.statSync.mockImplementation(() => {
+    (fs.statSync as jest.Mock).mockImplementation(() => {
       return { isDirectory: () => false, size: 10 };
     });
 
@@ -131,7 +126,7 @@ describe('push', () => {
         destination: 'test@v1',
         branchName: 'test',
         public: true,
-        'batch-id': '123',
+        'job-id': '123',
         'batch-size': 2,
       },
       ConfigFixture as any
@@ -139,9 +134,7 @@ describe('push', () => {
 
     expect(exitWithError).toBeCalledTimes(1);
     expect(exitWithError).toBeCalledWith(
-      `No organization provided, please use the right format: ${yellow(
-        '<@organization-id/api-name@api-version>'
-      )} or specify the 'organization' field in the config file.`
+      `No organization provided, please use --organization option or specify the 'organization' field in the config file.`
     );
   });
 
@@ -154,7 +147,7 @@ describe('push', () => {
         destination: 'my-api@1.0.0',
         branchName: 'test',
         public: true,
-        'batch-id': '123',
+        'job-id': '123',
         'batch-size': 2,
       },
       mockConfig
@@ -175,7 +168,7 @@ describe('push', () => {
     });
   });
 
-  it('push should work if destination not provided and api in config is provided', async () => {
+  it('push should work if destination not provided but api in config is provided', async () => {
     const mockConfig = {
       ...ConfigFixture,
       organization: 'test_org',
@@ -185,10 +178,9 @@ describe('push', () => {
     await handlePush(
       {
         upsert: true,
-        api: 'spec.json',
         branchName: 'test',
         public: true,
-        'batch-id': '123',
+        'job-id': '123',
         'batch-size': 2,
       },
       mockConfig
@@ -197,7 +189,7 @@ describe('push', () => {
     expect(redoclyClient.registryApi.pushApi).toBeCalledTimes(1);
   });
 
-  it('push should fail if destination and apis not provided', async () => {
+  it('push should fail if apis not provided', async () => {
     const mockConfig = { organization: 'test_org', apis: {} } as any;
 
     await handlePush(
@@ -205,7 +197,7 @@ describe('push', () => {
         upsert: true,
         branchName: 'test',
         public: true,
-        'batch-id': '123',
+        'job-id': '123',
         'batch-size': 2,
       },
       mockConfig
@@ -217,6 +209,49 @@ describe('push', () => {
     );
   });
 
+  it('push should fail if destination not provided', async () => {
+    const mockConfig = { organization: 'test_org', apis: {} } as any;
+
+    await handlePush(
+      {
+        upsert: true,
+        api: 'api.yaml',
+        branchName: 'test',
+        public: true,
+        'job-id': '123',
+        'batch-size': 2,
+      },
+      mockConfig
+    );
+
+    expect(exitWithError).toBeCalledTimes(1);
+    expect(exitWithError).toHaveBeenLastCalledWith(
+      'No destination provided, please use --destination option to provide destination.'
+    );
+  });
+
+  it('push should fail if destination format is not valid', async () => {
+    const mockConfig = { organization: 'test_org', apis: {} } as any;
+
+    await handlePush(
+      {
+        upsert: true,
+        destination: 'name/v1',
+        branchName: 'test',
+        public: true,
+        'job-id': '123',
+        'batch-size': 2,
+      },
+      mockConfig
+    );
+
+    expect(exitWithError).toHaveBeenCalledWith(
+      `Destination argument value is not valid, please use the right format: ${yellow(
+        '<api-name@api-version>'
+      )}`
+    );
+  });
+
   it('push should work and encode name with spaces', async () => {
     const encodeURIComponentSpy = jest.spyOn(global, 'encodeURIComponent');
 
@@ -232,7 +267,7 @@ describe('push', () => {
         destination: 'my test api@v1',
         branchName: 'test',
         public: true,
-        'batch-id': '123',
+        'job-id': '123',
         'batch-size': 2,
       },
       mockConfig
@@ -248,7 +283,7 @@ describe('transformPush', () => {
     const cb = jest.fn();
     transformPush(cb)(
       {
-        maybeApiOrDestination: 'openapi.yaml',
+        api: 'openapi.yaml',
         maybeDestination: '@testing_org/main@v1',
       },
       {} as any
@@ -265,7 +300,7 @@ describe('transformPush', () => {
     const cb = jest.fn();
     transformPush(cb)(
       {
-        maybeApiOrDestination: 'openapi.yaml',
+        api: 'openapi.yaml',
         maybeDestination: '@testing_org/main@v1',
         maybeBranchName: 'other',
       },
@@ -284,7 +319,7 @@ describe('transformPush', () => {
     const cb = jest.fn();
     transformPush(cb)(
       {
-        maybeApiOrDestination: 'openapi.yaml',
+        api: 'openapi.yaml',
         maybeDestination: '@testing_org/main@v1',
         maybeBranchName: 'other',
         branch: 'priority-branch',
@@ -304,7 +339,7 @@ describe('transformPush', () => {
     const cb = jest.fn();
     transformPush(cb)(
       {
-        maybeApiOrDestination: '@testing_org/main@v1',
+        api: '@testing_org/main@v1',
       },
       {} as any
     );
@@ -315,11 +350,26 @@ describe('transformPush', () => {
       {}
     );
   });
+  it('should work for a api only', () => {
+    const cb = jest.fn();
+    transformPush(cb)(
+      {
+        api: 'test.yaml',
+      },
+      {} as any
+    );
+    expect(cb).toBeCalledWith(
+      {
+        api: 'test.yaml',
+      },
+      {}
+    );
+  });
   it('should accept aliases for the old syntax', () => {
     const cb = jest.fn();
     transformPush(cb)(
       {
-        maybeApiOrDestination: 'alias',
+        api: 'alias',
         maybeDestination: '@testing_org/main@v1',
       },
       {} as any
@@ -332,6 +382,29 @@ describe('transformPush', () => {
       {}
     );
   });
+  it('should use --job-id option firstly', () => {
+    const cb = jest.fn();
+    transformPush(cb)(
+      {
+        'batch-id': 'b-123',
+        'job-id': 'j-123',
+        api: 'test',
+        maybeDestination: 'main@v1',
+        branch: 'test',
+        destination: 'main@v1',
+      },
+      {} as any
+    );
+    expect(cb).toBeCalledWith(
+      {
+        'job-id': 'j-123',
+        api: 'test',
+        branchName: 'test',
+        destination: 'main@v1',
+      },
+      {}
+    );
+  });
   it('should accept no arguments at all', () => {
     const cb = jest.fn();
     transformPush(cb)({}, {} as any);