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: add output as a per-API configuration option #1708

Merged
merged 14 commits into from
Sep 6, 2024

Conversation

tatomyr
Copy link
Contributor

@tatomyr tatomyr commented Sep 3, 2024

What/Why/How?

Added support for the output option in the per-API configuration. This adds the possibility to store bundled files for all apis that have the option specified.

For example, for the following redocly.yaml:

apis:
   v1: 
     root: openapi1.yaml
     output: dist/v1.yaml
   v2: 
     root: openapi2.yaml
     output: dist/v2.yaml

Executing redocly bundle will store bundled files under the dist/ directory.
Executing redocly bundle openapi1.yaml will result in printing only the openapi1 bundle to stdout.

Reference

Resolves #1579

Testing

Screenshots (optional)

Check yourself

  • Code changed? - Tested with redoc/reference-docs/workflows (internal)
  • All new/updated code is covered with tests
  • New package installed? - Tested in different environments (browser/node)

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

@tatomyr tatomyr self-assigned this Sep 3, 2024
Copy link

changeset-bot bot commented Sep 3, 2024

🦋 Changeset detected

Latest commit: 3878eb6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@redocly/cli Minor
@redocly/openapi-core Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Sep 3, 2024

Command Mean [ms] Min [ms] Max [ms] Relative
redocly lint packages/core/src/benchmark/benches/rebilly.yaml 980.0 ± 11.3 964.6 1006.4 1.00
redocly-next lint packages/core/src/benchmark/benches/rebilly.yaml 986.5 ± 21.2 957.8 1011.9 1.01 ± 0.02

Copy link
Contributor

github-actions bot commented Sep 3, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 78.35% 4899/6253
🟡 Branches
67.01% (+1.09% 🔼)
2021/3016
🟡 Functions 72.62% 809/1114
🟡 Lines 78.64% 4621/5876
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟡 core/src/utils.ts 76.28%
60.76% (-1.08% 🔻)
76% (-1.55% 🔻)
77.46%

Test suite run success

793 tests passing in 118 suites.

Report generated by 🧪jest coverage report action from 3878eb6

@tatomyr tatomyr requested a review from lornajane September 3, 2024 12:57
@tatomyr tatomyr marked this pull request as ready for review September 3, 2024 13:40
@tatomyr tatomyr requested review from a team as code owners September 3, 2024 13:40
@tatomyr tatomyr force-pushed the feat/add-per-api-output-for-bundle branch 2 times, most recently from ebf4efd to 6617f1a Compare September 4, 2024 12:27
@tatomyr tatomyr force-pushed the feat/add-per-api-output-for-bundle branch 3 times, most recently from ef129f3 to bbde7e8 Compare September 4, 2024 13:52
Copy link
Contributor

@lornajane lornajane left a comment

Choose a reason for hiding this comment

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

The feature behaves differently depending if we're bundling one API or many, which I did not expect. I can tell from the documentation that this is an intentional exception - but I don't know why we did this. In every other context, the API is linted/bundled the same way whether it's done alone by alias, or others are done at the same time. With this configuration, the bundle command will apply per-api decorators, but not per-api output, which seems wrong.

The command line parameter should still take priority if it's supplied, but the configuration should apply if it exists, regardless of whether there are other APIs being bundled at the same time or not.

.changeset/afraid-melons-sparkle.md Outdated Show resolved Hide resolved
docs/commands/bundle.md Outdated Show resolved Hide resolved
docs/commands/bundle.md Outdated Show resolved Hide resolved
docs/commands/bundle.md Show resolved Hide resolved
docs/commands/bundle.md Outdated Show resolved Hide resolved
@tatomyr
Copy link
Contributor Author

tatomyr commented Sep 5, 2024

@lornajane do you mean that having this redocy.yaml:

apis:
  main@v1:
    root: openapi.yaml
    output: bunlded.yaml

and running redocly bundle main@v1 it should store it to bundled.yaml?

@lornajane
Copy link
Contributor

Yes, running redocly bundle [alias] and running redocly bundle should do the same thing for the API named by [alias].

@tatomyr
Copy link
Contributor Author

tatomyr commented Sep 5, 2024

@lornajane but how would you print it out to the stdout for the alias if needed?

@lornajane
Copy link
Contributor

Oh, of course. You can't use the filename without silently picking up the alias :( I guess another alias, or a special value for --output

@tatomyr
Copy link
Contributor Author

tatomyr commented Sep 5, 2024

@lornajane another alias means you have to maintain consistency between those two (and remember twice more names too). And a special output value could collide with a filename (users are quite creative when it comes to naming things!).

@tatomyr
Copy link
Contributor Author

tatomyr commented Sep 5, 2024

I believe this option is mainly aimed for bundling and storing large amount of apis which is quite tedious to do via the command line. So that’s where the ‘output’ really comes in handy. If a user wants to bundle one alias, it’s not hard to specify the output destination as well. I hope that seeing the output, users will be able to figure out the behaviour.

@lornajane
Copy link
Contributor

The current behaviour is unexpected - test it alone and you get one behaviour. Run it in a batch and it does something different. I'm not comfortable with us shipping it as it is.

@tatomyr tatomyr force-pushed the feat/add-per-api-output-for-bundle branch from 3e2eb24 to e414930 Compare September 5, 2024 10:32
@tatomyr
Copy link
Contributor Author

tatomyr commented Sep 5, 2024

@lornajane changed the behaviour. Now if you bundle an alias, it will take the output from the config if it exists.

@lornajane
Copy link
Contributor

Everything here looks great to me, but I was surprised that I could bundle a file by name without it picking up the alias magically (since the file is mentioned in the config file). So I like everything here, but I wanted to mention it in case the change was not intentional.

@tatomyr tatomyr force-pushed the feat/add-per-api-output-for-bundle branch from 153de2a to 479e1aa Compare September 5, 2024 14:58
Copy link
Member

@RomanHotsiy RomanHotsiy left a comment

Choose a reason for hiding this comment

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

Left one comment, otherwise looks good.

packages/cli/src/utils/miscellaneous.ts Show resolved Hide resolved
@tatomyr tatomyr force-pushed the feat/add-per-api-output-for-bundle branch from 88fd5ef to f949ef9 Compare September 6, 2024 09:37
@tatomyr tatomyr requested a review from RomanHotsiy September 6, 2024 10:10
@tatomyr tatomyr merged commit e1ddf8e into main Sep 6, 2024
31 checks passed
@tatomyr tatomyr deleted the feat/add-per-api-output-for-bundle branch September 6, 2024 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add output as a per-API configuration option
3 participants