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

chore: bump openapi #200

Merged
merged 2 commits into from
Feb 15, 2024
Merged

chore: bump openapi #200

merged 2 commits into from
Feb 15, 2024

Conversation

mfw78
Copy link
Contributor

@mfw78 mfw78 commented Feb 14, 2024

This PR:

  1. Bumps the openapi.yml to the latest
  2. Regenerates the associated models
  3. Patch level bumps the version

Kudos to @alfetopito 🙇

@mfw78 mfw78 requested a review from a team February 14, 2024 17:48
@coveralls
Copy link
Collaborator

coveralls commented Feb 14, 2024

Coverage Status

coverage: 78.851%. remained the same
when pulling 5fcb1de on bump-openapi
into 9d9abb5 on main.

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Thank you!
A few comments/requests.

Other than the version discussion, the rest is not actionable here.

*/
export type Surplus = {
factor: number;
max_volume_factor: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Request for backed: camelCase please?

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "@cowprotocol/cow-sdk",
"version": "5.0.0",
"version": "5.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure a patch version bump is adequate.
At least a minor version bump?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, was wondering about this.

@@ -27,7 +27,7 @@
"prepare": "npm run build",
"prepublishOnly": "npm test && npm run lint",
"graphql:codegen": "graphql-codegen --config graphql-codegen.yml",
"swagger:codegen": " openapi --input https://raw.githubusercontent.com/cowprotocol/services/v2.227.1/crates/orderbook/openapi.yml --output src/order-book/generated --exportServices false --exportCore false",
"swagger:codegen": " openapi --input https://raw.githubusercontent.com/cowprotocol/services/v2.245.1/crates/orderbook/openapi.yml --output src/order-book/generated --exportServices false --exportCore false",
Copy link
Contributor

Choose a reason for hiding this comment

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

why not getting from latest from master the openapi?

Copy link
Contributor Author

@mfw78 mfw78 Feb 14, 2024

Choose a reason for hiding this comment

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

I don't mind, I'm only following the convention that was here previously.

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 is nice that we use the latest. If swagger becomes autogenerated, and this uses the latest, we will notice when there's unmapped errors if we touch the lib

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm almost sure I suggested to use release rather than master to have a better control/tracking of where the current version comes from.
If pointing to a branch, it's harder to tell which commit was used as a reference.

If we don't want to use a release, should we use a commit on main branch instead?
This way the update is intentional and clear.

Copy link
Contributor

@anxolin anxolin Feb 15, 2024

Choose a reason for hiding this comment

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

I'm fine with pointing to a version, I actually think is safer and agree with it being more reproducible.
My only concern is, we won't remember to update from time to time, if this doesn't bother us on commit.

One alternative is to make a check if we use the latest version.

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Approve.

I would love we can automatically re-generate things when the backend updates the swagger, but this is not part of this PR, so approving

@mfw78 mfw78 merged commit 3d3e9f5 into main Feb 15, 2024
9 checks passed
@mfw78 mfw78 deleted the bump-openapi branch February 15, 2024 19:14
@github-actions github-actions bot locked and limited conversation to collaborators Feb 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants