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: lint arazzo test descriptions #1601

Merged
merged 9 commits into from
Jul 10, 2024
Merged

Conversation

DmitryAnansky
Copy link
Contributor

@DmitryAnansky DmitryAnansky commented Jun 26, 2024

What/Why/How?

Add Arazzo validation to the lint cli command.
Now it is possible to lint Arazzo description file by running redocly lint museum.yaml

Reference

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

Copy link

changeset-bot bot commented Jun 26, 2024

🦋 Changeset detected

Latest commit: 54bce55

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

This PR includes changesets to release 2 packages
Name Type
@redocly/openapi-core Minor
@redocly/cli 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 Jun 26, 2024

Command Mean [ms] Min [ms] Max [ms] Relative
redocly lint packages/core/src/benchmark/benches/rebilly.yaml 992.0 ± 17.2 962.8 1016.6 1.00
redocly-next lint packages/core/src/benchmark/benches/rebilly.yaml 1004.4 ± 20.6 980.1 1044.9 1.01 ± 0.03

Copy link
Contributor

github-actions bot commented Jun 26, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 77.22% 4565/5912
🟡 Branches 67.18% 2512/3739
🟡 Functions 70.72% 751/1062
🟡 Lines 77.45% 4300/5552
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / arazzo.ts
100% 100% 100% 100%
🟢
... / index.ts
100% 100% 100% 100%
🟢
... / index.ts
100% 100% 100% 100%
🟢
... / index.ts
100% 100% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟡
... / config.ts
65.27% (-5.05% 🔻)
67.28% (-10.02% 🔻)
61.29% (-6.57% 🔻)
65.77% (-4.23% 🔻)
🟢
... / oas-types.ts
93.62% (-3.88% 🔻)
85.71% (-7.62% 🔻)
100%
93.62% (-3.88% 🔻)
🟢
... / config-resolvers.ts
84% 75.72% 100% 84.62%
🟡 core/src/bundle.ts
75% (-3.52% 🔻)
62.71% (-7.76% 🔻)
94.12%
75% (-3.52% 🔻)
🟢 core/src/index.ts 100% 100% 8.93% 100%

Test suite run success

750 tests passing in 105 suites.

Report generated by 🧪jest coverage report action from 54bce55

packages/cli/src/utils/miscellaneous.ts Outdated Show resolved Hide resolved
packages/core/src/lint.ts Outdated Show resolved Hide resolved
docs/commands/lint.md Outdated Show resolved Hide resolved
@DmitryAnansky DmitryAnansky marked this pull request as ready for review June 27, 2024 15:15
@DmitryAnansky DmitryAnansky requested review from a team as code owners June 27, 2024 15:15
Copy link
Contributor

@tatomyr tatomyr left a comment

Choose a reason for hiding this comment

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

Left a few comments. Otherwise looks good.

museum.yaml Outdated Show resolved Hide resolved
packages/cli/src/index.ts Outdated Show resolved Hide resolved
packages/core/src/config/__tests__/config.test.ts Outdated Show resolved Hide resolved
packages/core/src/config/config.ts Show resolved Hide resolved
packages/core/src/oas-types.ts Show resolved Hide resolved
@DmitryAnansky DmitryAnansky force-pushed the feat/linting-arazzo-files branch 2 times, most recently from 984b545 to 2fde553 Compare July 1, 2024 13:44
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.

It's a small thing, but we should also update the CLI help output to add arazzo to the lint command description.

Linting some of the Arazzo examples from the main project, I get some errors "The field in must be present on this level." but the entry looks like this:

workflowId: place-order
parameters:
  - name: pet_id
    value: $steps.find-pet.outputs.my_pet_id

Looking at the spec document for parameters, I think the "in" isn't required when it's a reference to something that's already in the scope of an Arazzo workflow.

@DmitryAnansky
Copy link
Contributor Author

It's a small thing, but we should also update the CLI help output to add arazzo to the lint command description.

Linting some of the Arazzo examples from the main project, I get some errors "The field in must be present on this level." but the entry looks like this:

workflowId: place-order
parameters:
  - name: pet_id
    value: $steps.find-pet.outputs.my_pet_id

Looking at the spec document for parameters, I think the "in" isn't required when it's a reference to something that's already in the scope of an Arazzo workflow.

Thanks Lorna,
We have a task for this issue => https://github.com/orgs/Redocly/projects/51/views/1?pane=issue&itemId=65948722

@lornajane
Copy link
Contributor

We don't have support for references in step parameters (or I'm doing something wrong! Testing the pet coupons example ), will we list that as not supported, or should we add it? https://github.com/OAI/Arazzo-Specification/blob/main/versions/1.0.0.md#step-object

Copy link
Contributor

@tatomyr tatomyr left a comment

Choose a reason for hiding this comment

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

Almost there. A few more comments.

packages/core/src/config/types.ts Show resolved Hide resolved
packages/core/src/decorators/arazzo/index.ts Show resolved Hide resolved
packages/core/src/rules/arazzo/index.ts Show resolved Hide resolved
packages/core/src/types/arazzo.ts Outdated Show resolved Hide resolved
docs/commands/lint.md Outdated Show resolved Hide resolved
__tests__/commands.test.ts Outdated Show resolved Hide resolved
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.

Very nice, works well with the samples in the main project (and I have some tiny fixes to send them that I found using this tool, so it's already proven its value!. Nice work!

@DmitryAnansky DmitryAnansky merged commit 599b5fa into main Jul 10, 2024
33 checks passed
@DmitryAnansky DmitryAnansky deleted the feat/linting-arazzo-files branch July 10, 2024 09:08
jeremyfiel pushed a commit to jeremyfiel/redocly-cli that referenced this pull request Jul 16, 2024
* feat: lint arazzo test descriptions
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.

5 participants