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.

Sign up for GitHub

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

Jump to bottom

Add a new overlay command to be able to apply OpenAPI overlays on given API document #553

Merged
merged 3 commits into from
Mar 18, 2024
Merged

Add a new overlay command to be able to apply OpenAPI overlays on given API document #553

merged 3 commits into from
Mar 18, 2024

Conversation

paulRbr
Copy link
Member

@paulRbr paulRbr commented Dec 28, 2023 •
edited
Loading

Together with the guide written by Phil on OpenAPI overlays, we thought it would be a good idea to release a new bump overlay command in the CLI in order to test the new OpenAPI Overlay specification.

This WIP change is based on @lornajane's work on overlays with a few changes here and there.

I've published the resulting built package from this branch as a beta version on npmjs registry so you can test a beta version of Bump CLI installable with:

npm install bump-cli@beta

Example usage

  • Wrong overlay file
> bump overlay ../bump/doc/api/v1/openapi.v3.yml ../bump/doc/api/v1/openapi.v3.yml
 ›   Error: ../bump/doc/api/v1/openapi.v3.yml does not look like an OpenAPI overlay
  • Correct overlay file
bump overlay ../bump/doc/api/v1/openapi.v3.yml ../bump/doc/api/v1/overlay.yaml > ../bump/doc/api/v1/openapi.v3.overlayed.yml
bump deploy  ../bump/doc/api/v1/openapi.v3.overlayed.yml

Note: the input API document file name is used to detect the target output overlayed document (Json or Yaml)

  • With the explicit --out argument
bump overlay ../bump/doc/api/v1/openapi.v3.yml ../bump/doc/api/v1/overlay.yaml --out ../bump/doc/api/v1/openapi.v3.overlayed.yml
  • Applying an overlay right before a deployment
bump deploy ../bump/doc/api/v1/openapi.v3.yml --overlay ../bump/doc/api/v1/overlay.yaml --doc my-doc --token --my-token

@paulRbr paulRbr self-assigned this Dec 28, 2023
@paulRbr paulRbr mentioned this pull request Dec 28, 2023
Merged
@philsturgeon
Copy link

Can we pop in a Thanks section on the README and put Lorna Mitchel on there?

We've done this for Spectral, Prism, and I recommend it for any tool which has substantial contribution from a single contributor, like this.

https://github.com/stoplightio/spectral#-thanks

@paulRbr paulRbr force-pushed the overlay-testing branch 3 times, most recently from 1bed61c to 5715cd8 Compare December 28, 2023 16:43
@paulRbr paulRbr changed the title WIP: Add a new overlay command to be able to apply OpenAPI overlays on given API document Add a new overlay command to be able to apply OpenAPI overlays on given API document Jan 9, 2024
@paulRbr paulRbr mentioned this pull request Feb 26, 2024
Closed
@paulRbr paulRbr removed the request for review from scharrier February 26, 2024 11:43
@paulRbr paulRbr force-pushed the overlay-testing branch 3 times, most recently from 4e2a7bd to 017aab0 Compare March 15, 2024 10:15
@paulRbr paulRbr marked this pull request as draft March 15, 2024 10:16
@paulRbr paulRbr marked this pull request as ready for review March 15, 2024 10:16
@paulRbr paulRbr requested a review from scharrier March 15, 2024 10:16
@paulRbr paulRbr force-pushed the overlay-testing branch 2 times, most recently from 7e732e3 to 5d1408e Compare March 15, 2024 11:27
scharrier
scharrier reviewed Mar 15, 2024
View reviewed changes
Copy link
Member

@scharrier scharrier left a comment

Choose a reason for hiding this comment

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

It's a great one! I cant wait to release it, and allow our users to start using OpenAPI overlays.

I asked a few questions, nothing blocking here. I let you answer them, and will do another pass then.

Thanks, @paulRbr <3

src/core/deploy.ts Outdated Show resolved Hide resolved
src/core/deploy.ts Outdated Show resolved Hide resolved
src/core/deploy.ts Outdated Show resolved Hide resolved
src/core/overlay.ts Outdated Show resolved Hide resolved
src/core/overlay.ts Outdated Show resolved Hide resolved
src/core/overlay.ts Outdated
Comment on lines 31 to 39
// WIP @github.com/lornajane/openapi-overlays-js
//
// I couldn't get the upstream lib to be imported properly due to
// some issues with ESM module imports so this is method was copied
// from github.com/lornajane/openapi-overlays-js and has been
// adapted to make our Typescript build happy.
//
// If you make any changes here, PLEASE ALSO MAKE THEM UPSTREAM.
applyOverlay(spec: APIDefinition, overlay: OpenAPIOverlay): APIDefinition {
Copy link
Member

Choose a reason for hiding this comment

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

It feel like this method could be a bit enhanced by extracting smaller methods, having a case to split the different actions, etc...

It can be done later, but I think it would be a great idea to make it a bit clearer. WDYT?

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 point here was to stay as close as possible from the original code @github.com/lornajane/openapi-overlays-js. The code will soon be available as a library and we will be able to build a common ground in a dedicated package which will remove this code from our CLI.

I'd prefer to keep it as is for now, if you don't mind?

src/definition.ts Show resolved Hide resolved
src/flags.ts Show resolved Hide resolved
test/commands/overlay.test.ts Show resolved Hide resolved
@paulRbr
Copy link
Member Author

paulRbr commented Mar 18, 2024

@scharrier following your review, I've moved things around to make it clearer (I think). Basically all the “command” related feature is in the new commands/overlay.ts file while all the things around the definition (format, applying overlay...) is in the APIdefinition model.

I think it makes more sense like this. Let me know if it suits you!

@paulRbr paulRbr requested a review from scharrier March 18, 2024 08:23
scharrier
scharrier approved these changes Mar 18, 2024
View reviewed changes
Copy link
Member

@scharrier scharrier left a comment

Choose a reason for hiding this comment

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

I think we have something that we can release, here! 👏
I left a few questions/feedback, I let you decide on how to handle them.

src/commands/overlay.ts Outdated Show resolved Hide resolved
src/definition.ts Outdated Show resolved Hide resolved
src/flags.ts Show resolved Hide resolved
test/commands/overlay.test.ts Show resolved Hide resolved
@paulRbr
cmd: adding an 'overlay' command to be able to apply OAS overlays
f85f266 
This is a work in progress.. but I have troubles to import the lib
from https://github.com/lornajane/openapi-overlays-js and I don't
understand why 🤯
@paulRbr
deploy: adding an --overlay flag to the deploy command
1fa0fdc 
This commit adds an optional `--overlay` command to the existing
command `bump deploy` to be ably to apply an overlay on the target
API document before deploying.

This parameter only works for a single file deploy (not compatible
with a DIRECTORY + hub deploy)
@paulRbr
tests: add example overlay targetting objects with field condition
d0f3a7c 
This commit adds a case where the overlay is targetting objects with a
condition on the object (here objects where `x-beta: true` is set).
@paulRbr paulRbr merged commit ca88767 into main Mar 18, 2024
9 checks passed
@paulRbr paulRbr mentioned this pull request Mar 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scharrier scharrier scharrier approved these changes

Assignees

@paulRbr paulRbr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@paulRbr @philsturgeon @scharrier