feat: update push command (#1137)

Co-authored-by: Andrew Tatomyr <andrew.tatomyr@redocly.com>
Co-authored-by: Lorna Mitchell <lorna.mitchell@redocly.com>

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 as jest.Mock).mockImplementation(() => {
-    fs.statSync.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(
+      `No organization provided, please use --organization option or specify the 'organization' field in the config file.`
-        '<@organization-id/api-name@api-version>'
-      )} 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);

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(
+      `No organization provided, please use --organization option or specify the 'organization' field in the config file.`
-        '<@organization-id/api-name@api-version>'
-      )} 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(
+        exitWithError(
-          version
+          `The definition version ${blue(
-        )} does not exist in organization ${blue(organizationId)}!\n${yellow(
+            `${name}@${version}`
-          'Suggestion:'
+          )} does not exist in organization ${blue(organizationId)}!\n${yellow(
-        )} please use ${blue('-u')} or ${blue('--upsert')} to create definition.
+            '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'> & {
+type BarePushArgs = Omit<PushOptions, 'destination' | 'branchName'> & {
-  maybeApiOrDestination?: string;
   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 (maybeBranchName) {
+    if (batchId) {
       process.stderr.write(
         yellow(
-          'Deprecation warning: Do not use the third parameter as a branch name. Please use a separate --branch option instead.'
+          `The ${red('batch-id')} option is deprecated. Please use ${green('job-id')} instead.\n\n`
         )
       );
     }
-    const api = maybeDestination ? maybeApiOrDestination : undefined;
+
-    const destination = maybeDestination || maybeApiOrDestination;
+    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.\n\n'
+        )
+      );
+    }
+
+    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
     );

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' },
+          organization: {
-          upsert: { type: 'boolean', alias: 'u' },
+            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);