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: update push and push-status commands #1481

Merged
merged 42 commits into from
Apr 25, 2024

Conversation

lesyk-lesyk
Copy link
Contributor

@lesyk-lesyk lesyk-lesyk commented Mar 14, 2024

What/Why/How?

This PR refactors push and push-status commands.
After this PR, these commands will return the necessary values for use in other places (like GH action).
Stylish output was not changed.

Reference

https://github.com/orgs/Redocly/projects/42?pane=issue&itemId=54045541

Testing

Unit tests, local.

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 Mar 14, 2024

🦋 Changeset detected

Latest commit: 73e846a

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 Mar 14, 2024

Command Mean [ms] Min [ms] Max [ms] Relative
redocly lint packages/core/src/benchmark/benches/rebilly.yaml 948.5 ± 20.2 925.9 979.0 1.00
redocly-next lint packages/core/src/benchmark/benches/rebilly.yaml 956.0 ± 19.3 938.9 996.0 1.01 ± 0.03

Copy link
Contributor

github-actions bot commented Mar 14, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 77.05% 4451/5777
🟡 Branches 67.48% 2447/3626
🟡 Functions 70.73% 742/1049
🟡 Lines 77.24% 4185/5418
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / utils.ts
100% 87.5% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟡 core/src/utils.ts
76.56% (-1.04% 🔻)
63.64%
73.91% (-3.36% 🔻)
77.12%
🟢 core/src/index.ts 100% 100% 7.27% 100%

Test suite run success

734 tests passing in 102 suites.

Report generated by 🧪jest coverage report action from 73e846a

@tatomyr tatomyr changed the title Feat/update push status command feat: update push status command Mar 14, 2024
@lesyk-lesyk lesyk-lesyk changed the title feat: update push status command feat: update push and push-status commands Mar 27, 2024
@lesyk-lesyk lesyk-lesyk marked this pull request as ready for review March 27, 2024 12:19
@lesyk-lesyk lesyk-lesyk requested review from a team as code owners March 27, 2024 12:19
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.

Couple of suggestions, the docs are currently in a private repo but the pull request for the docs updates should be linked from this pull request.

@@ -179,6 +180,11 @@ yargs
description: 'Maximum execution time in seconds.',
type: 'number',
},
'ignore-deployment-failures': {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be continue-on-error or something along those likes (github uses continue-on-error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe continue-on-deployment-error will be better?

Copy link
Member

@adamaltman adamaltman Apr 23, 2024

Choose a reason for hiding this comment

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

We use the term deploy and not deployment universally. I'm a stickler for sticking to domain language so I think we shouldn't use the word deployment whenever we can avoid it. @lesyk-lesyk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to continue-on-deploy-failures, thanks! @adamaltman

packages/cli/src/index.ts Outdated Show resolved Hide resolved
@@ -340,6 +346,11 @@ yargs
type: 'boolean',
default: false,
},
'ignore-deployment-failures': {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest that we might rename this option as above

Copy link
Contributor Author

@lesyk-lesyk lesyk-lesyk Apr 16, 2024

Choose a reason for hiding this comment

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

I have renamed this parameter to continue-on-deploy-failures

packages/cli/src/index.ts Outdated Show resolved Hide resolved
@@ -179,6 +180,11 @@ yargs
description: 'Maximum execution time in seconds.',
type: 'number',
},
'ignore-deployment-failures': {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also it will need a docs update (in a different repo at the moment I know, it's fine to create the PR or a ticket on the monorepo and link to this PR for now)

Copy link
Contributor Author

@lesyk-lesyk lesyk-lesyk Apr 16, 2024

Choose a reason for hiding this comment

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

I have created a separate task for docs. Thanks.

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.

Had a quick look. Will review in more details a bit later.

.changeset/heavy-swans-sin.md Outdated Show resolved Hide resolved
packages/cli/src/cms/commands/push-status.ts Show resolved Hide resolved
@lesyk-lesyk lesyk-lesyk force-pushed the feat/update-push-status-command branch 2 times, most recently from d9d4d34 to 0101846 Compare April 15, 2024 12:12
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 minor comments.

packages/cli/src/cms/commands/push-status.ts Outdated Show resolved Hide resolved
packages/cli/src/cms/commands/push.ts Outdated Show resolved Hide resolved
packages/cli/src/cms/commands/utils.ts Outdated Show resolved Hide resolved
@lesyk-lesyk lesyk-lesyk force-pushed the feat/update-push-status-command branch 2 times, most recently from ac40017 to 9edd6b3 Compare April 23, 2024 14:40
resources/museum.yaml Outdated Show resolved Hide resolved
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.

Looks good. Thank you!

@lesyk-lesyk lesyk-lesyk force-pushed the feat/update-push-status-command branch from f7dd65c to 1513c8e Compare April 24, 2024 05:22
@lesyk-lesyk lesyk-lesyk merged commit 4cf08db into main Apr 25, 2024
30 checks passed
@lesyk-lesyk lesyk-lesyk deleted the feat/update-push-status-command branch April 25, 2024 11:42
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