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

fix: make schemas consistent with API #348

Merged
merged 1 commit into from
Dec 12, 2023
Merged

fix: make schemas consistent with API #348

merged 1 commit into from
Dec 12, 2023

Conversation

phm07
Copy link
Contributor

@phm07 phm07 commented Dec 6, 2023

In some places the schema definitions are not consistent with the API documentation, for example properties may not have pointer receivers even though they are marked as optional in the documentation. This results in inconsistent JSON when converting JSON -> Schema -> JSON (as seen for example in hetznercloud/cli#461, where one output is the exact API response and one is serialized and then converted back to JSON).

This PR aims to fix this issue by making the schema definitions consistent.

@phm07 phm07 self-assigned this Dec 6, 2023
@phm07 phm07 force-pushed the schema-consistency branch 2 times, most recently from d8d5914 to 0b42af2 Compare December 6, 2023 13:34
@phm07 phm07 force-pushed the schema-consistency branch 2 times, most recently from 48ce359 to 007ce46 Compare December 12, 2023 08:12
@phm07 phm07 force-pushed the schema-consistency branch from 007ce46 to b9a610d Compare December 12, 2023 09:45
@phm07 phm07 marked this pull request as ready for review December 12, 2023 10:03
@phm07 phm07 requested a review from a team as a code owner December 12, 2023 10:03
@jooola
Copy link
Member

jooola commented Dec 12, 2023

Isn't this a breaking change ? Or at least a feature?

@phm07
Copy link
Contributor Author

phm07 commented Dec 12, 2023

The schema package is not guaranteed to be consistent between versions as it's intended for internal use

@jooola
Copy link
Member

jooola commented Dec 12, 2023

Alright, do we use it externally? Or could we consider moving it into a internal/schema package?

@phm07
Copy link
Contributor Author

phm07 commented Dec 12, 2023

We use them in hcloud/schema.go which is exported, so I don't think we can move them into an internal package

@apricote
Copy link
Member

We use it externally to implement the JSON output, as only the schema types contain the JSON tags

@apricote
Copy link
Member

apricote commented Dec 12, 2023

I did a search for the import path, and it looks like its only used in hcloud-go, cli and the test suites for HCCM, packer and cluster-api-provider-hetzner: https://sourcegraph.com/search?q=context:global+%22github.com/hetznercloud/hcloud-go/v2/hcloud/schema%22+-file:vendor/*+-repo:github.com/hetznercloud/hcloud-go+-repo:github.com/hetznercloud/cli&patternType=standard&sm=1&groupBy=repo

@phm07 phm07 changed the title refactor: make schemas consistent with API fix: make schemas consistent with API Dec 12, 2023
@phm07
Copy link
Contributor Author

phm07 commented Dec 12, 2023

I made it a fix so it shows up in the changelog. I think that should be enough

@phm07 phm07 merged commit b0d7055 into main Dec 12, 2023
5 checks passed
@phm07 phm07 deleted the schema-consistency branch December 12, 2023 11:21
phm07 pushed a commit that referenced this pull request Dec 12, 2023
🤖 I have created a release *beep* *boop*
---


##
[2.5.0](v2.4.0...v2.5.0)
(2023-12-12)


### Features

* add conversion methods from schema to hcloud objects
([#343](#343))
([6feda4d](6feda4d))
* add interfaces for client structs
([#342](#342))
([4f9390f](4f9390f))
* add missing properties
([#349](#349))
([c8a28d0](c8a28d0))
* **error:** include http response in api errors
([#320](#320))
([9558239](9558239))


### Bug Fixes

* make schemas consistent with API
([#348](#348))
([b0d7055](b0d7055))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants