Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: creation of new openapi:reduce command to reduce large OpenAPI definitions #572

Merged
merged 16 commits into from
Aug 19, 2022

Conversation

erunion
Copy link
Member

@erunion erunion commented Aug 19, 2022

🚥 Fix RM-5114

🧰 Changes

Earlier this week I ported the reduction functionality in oas-reducer, a rad library I built last year to reduce an OpenAPI definition, over into oas with the intention of porting the CLI side of it in here as a new command. This is that! Why? oas-reducer is cool as hell and deserves a larger audience1.

Thanks to the refactoring going on in #570 developing and testing this was a goddamned dream.

ℹ️ After we eventually publish a new major release of rdme for all the work we've been doing I'll formally deprecate oas-reducer in NPM and archive the repo.

🧬 QA & Testing

Here's what it looks like when you supply it a spec2:

Screen.Recording.2022-08-18.at.11.03.07.PM.mov

And with the work that's been happening with the promptOas library, and its file selection work, it looks great when you don't give it a spec:

Screen.Recording.2022-08-18.at.11.08.56.PM.mov

Footnotes

  1. It especially deserves to be core to rdme and the whole rdme openapi flow if CX or Sales occasionally recommend it to customers to chunk up huge OAS files into smaller ones so they can be uploaded within our smol Heroku timeouts.

  2. The error at the end of this video is because I didn't select a method to filter down that path by. I've since fixed this by adding a min: 1 to the methods question.

@erunion erunion added command:openapi Issues pertaining to the `openapi`, `validate`, `reduce`, or `swagger` commands enhancement New feature or request labels Aug 19, 2022
@erunion erunion changed the title feat: porting over the oas-reducer CLI into a new openapi:reduce command feat: creation of new openapi:reduce command to reduce large OpenAPI definitions Aug 19, 2022
@erunion erunion requested a review from kanadgupta August 19, 2022 06:21
Object {
"description": "Alias for \`rdme openapi\`. [deprecated]",
"name": "swagger",
"position": 2,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that this has always been showing up after validate in --help with them having the same position.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lots-of-yikes:

@@ -1,4 +1,4 @@
import type { CommandOptions } from '../lib/baseCommand';
import type { CommandOptions } from '../../lib/baseCommand';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this into src/cmds/openapi/index.ts because the openapi command group is a family now. :corona: :vindiesel:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh the v81 release notes are definitely going to be F&F-themed now

Footnotes

  1. also v8... multitudes!!!!

Base automatically changed from kanad/rm-5116-migrate-from-enquirer-to-prompts to main August 19, 2022 15:34
Copy link
Member

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looking good, most of my comments are minor! chatted briefly about this offline but I saw this in the tag selection (note the [object Object]):

image

__tests__/cmds/openapi/reduce.test.ts Show resolved Hide resolved
Object {
"description": "Alias for \`rdme openapi\`. [deprecated]",
"name": "swagger",
"position": 2,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lots-of-yikes:

@@ -1,4 +1,4 @@
import type { CommandOptions } from '../lib/baseCommand';
import type { CommandOptions } from '../../lib/baseCommand';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh the v81 release notes are definitely going to be F&F-themed now

Footnotes

  1. also v8... multitudes!!!!

src/cmds/openapi/reduce.ts Outdated Show resolved Hide resolved
src/cmds/openapi/reduce.ts Outdated Show resolved Hide resolved
src/lib/prepareOas.ts Show resolved Hide resolved
src/cmds/openapi/reduce.ts Show resolved Hide resolved
src/cmds/openapi/reduce.ts Outdated Show resolved Hide resolved
src/cmds/openapi/reduce.ts Outdated Show resolved Hide resolved
src/cmds/openapi/reduce.ts Show resolved Hide resolved
@erunion
Copy link
Member Author

erunion commented Aug 19, 2022

chatted briefly about this offline but I saw this in the tag selection (note the [object Object]):

image

Turns out this was because that spec has a {name: 'tag'} XML property in it and that was being picked up by the loose JSONPath query. I've added some filtering to filter these out:

Screen Shot 2022-08-19 at 10 39 06 AM

@erunion erunion requested a review from kanadgupta August 19, 2022 17:45
message: 'Enter the path to save your reduced API definition to:',
initial: () => {
const extension = path.extname(specPath);
return `${path.basename(specPath).split(extension)[0]}-reduced${extension}`;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This'll dynamically turn petstore.json or petstore.yaml into petstore-reduced.{ext}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:nice-rgb:

spinner.succeed(`${spinner.text} done! ✅`);
} catch (err) {
Command.debug(`err=${err.message}`);
throw err;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reducer throws exceptions for errors (eg. if you try to filter out too much) right now so instead of trying to capture and put those in some sort of "Yikes! We couldn't reduce this because...` error I'm just letting it do its thing.

Copy link
Member

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny nit but otherwise LGTM!

.filter(tag => {
// A spec may have a `name: tag` which this JSONPath query will pick up. Since that
// definitely isn't a tag we want here we need to filter them out.
return typeof tag === 'string';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

damn nice find... I wish there was an easy way to test this but can't really think of one 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not sure how to test what a prompt in prompts is rendering without factoring this work out into a separate small utility.

src/cmds/openapi/reduce.ts Outdated Show resolved Hide resolved
message: 'Enter the path to save your reduced API definition to:',
initial: () => {
const extension = path.extname(specPath);
return `${path.basename(specPath).split(extension)[0]}-reduced${extension}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:nice-rgb:

@erunion erunion merged commit 074c13e into main Aug 19, 2022
@erunion erunion deleted the feat/oas-reducer branch August 19, 2022 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command:openapi Issues pertaining to the `openapi`, `validate`, `reduce`, or `swagger` commands enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants