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

Versioning endpoints #50

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

Versioning endpoints #50

wants to merge 33 commits into from

Conversation

Sporiff
Copy link
Member

@Sporiff Sporiff commented Aug 27, 2023

This PR contains an initial pass at the documentation for the following endpoints:

/versions
/{major_version}/capabilities

The documentation broadly implements the points discussed at the last meeting

Copy link
Contributor

@JonOfUs JonOfUs left a comment

Choose a reason for hiding this comment

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

This looks very good! I have deployed this PR here:
https://versioning-spec--openpodcastapi-test-deploy.netlify.app
It should™ auto-update.

Comment on lines +65 to +72
"versions": {
"v1": {
"minor": 6
},
"v2": {
"minor": 0
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be an array, if I remember correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"versions": {
"v1": {
"minor": 6
},
"v2": {
"minor": 0
}
}
"versions": [
"v1": {
"minor": 6
},
"v2": {
"minor": 0
}
]

Copy link
Member Author

Choose a reason for hiding this comment

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

@JonOfUs This is actually very hard to achieve in an OpenAPI scheme. Since the key (major version) is an unknown value such as "v1", "v2", "v3", you need to put it in as an additional property. The whole thing is essentially a dictionary where you have a string key and an object value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be easier to also put the major version into the object? Or is it too difficult to read then?

{
  "versions": [
    {
      "major": 1,
      "minor": 6
    },
    {
      "major": 2,
      "minor": 0
    }
  ]
}

Or we just stick with "v1.2" and accept the string parsing, then we could also add release candidate versions with e.g. "rc1.6" or something like that for testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

@keunes @georgkrause Any preferences?

Copy link
Member Author

Choose a reason for hiding this comment

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

@georgkrause I think you're probably right. For versions, we probably only need to mention the major version support. We should rely on a capabilities endpoint to communicate what capabilities are implemented by the server, and the major version should only communicate changes to core endpoints.

Copy link
Member

@keunes keunes Nov 3, 2024

Choose a reason for hiding this comment

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

I am failing to consider any change that you wouldn't consider to be breaking, so you would never actually bump a minor version.

@georgkrause I think we foresaw the minor versions to be used for extended functionality (new, optional endpoints or fields). If we want, say, add the is_in_queue field on the episodes endpoint, is it worth a new major version? I guess not. But indeed this scenario would be covered with the capabilities end-point.)

major version should only communicate changes to core endpoints

@Sporiff I would say we'll also need a new version if there are breaking changes in optional endpoints. Imagine we would change the download_status field from boolean to ENUM without increasing the version, that wouldn't be clear from the capabilities endpoint and could break apps' functionality, no? (And if so, wouldn't minor versions be a better fit for this?)

I think for us we'll still want to have minor releases, and use git tags (specifically for the Open API specs file). But I guess that doing so doesn't mean we'd have to have those minor versions in the endpoint itself also.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@keunes

I think we foresaw the minor versions to be used for extended functionality (new, optional endpoints or fields). If we want, say, add the is_in_queue field on the episodes endpoint, is it worth a new major version? I guess not. But indeed this scenario would be covered with the capabilities end-point.)

Your example is indeed valid, so lets talk about! We could do it this way, however I am afraid of getting into a situation of huge complexity that makes it hard for app developers to use our API. If you need to run a check before talking to the API if the endpoint property is supported by the server (and maybe the server even has to check the clients capabilities in order to compile the response?), it gets pretty complex to develop against the API. It would be much easier if we had a set of API endpoints that is well defined and supported by each and every server. Any additional functionality can be pulled in by our extensions/optional endpoints.

We could also think about how this would be for the end users. I think the core functionality needs to be proved everywhere in order to avoid confusion (eg Why can this server sync my queue but the others don't but they do something else?).

Your example was the addition of a filter, it gets even more complicated if we talk about removing a filter, would you consider this to be a breaking change?

The only advantage of allowing minor updates and versions is that its more convenient for us: We don't need to extensively think and test before we cut a release, because we can just apply smaller changes to the API. However, I think we should take the task, take the time, design properly (which we already do) and have quite some beta-testing and implementation tests before releasing something. We can then focus on packaging a new major version of the API if required.

I think for us we'll still want to have minor releases, and use git tags (specifically for the Open API specs file). But I guess that doing so doesn't mean we'd have to have those minor versions in the endpoint itself also.

I think thats true. We can have minor releases in git, for example to improve documentation, change optional endpoints or even fix things that were broken.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Sporiff I would say we'll also need a new version if there are breaking changes in optional endpoints. Imagine we would change the download_status field from boolean to ENUM without increasing the version, that wouldn't be clear from the capabilities endpoint and could break apps' functionality, no? (And if so, wouldn't minor versions be a better fit for this?)

This is true and we probably need to start a discussion about optional endpoints. Maybe its not a good idea to think about required/optional endpoints. Maybe we should instead have a core API with versioning and then we can allow extensions to the API, that following their own versioning?

Copy link
Member

@keunes keunes Nov 4, 2024

Choose a reason for hiding this comment

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

Those are some interesting points @georgkrause – less of a quick decision than I thought it might be 😋 Maybe that could be a nice topic for Thursday, if you're able to join a meeting then?

Comment on lines +82 to +83
<version>v1.1</version>
<version>v2.0</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

We thought about putting the minor version into attributes here

src/content/docs/specs/versioning/index.mdx Show resolved Hide resolved
keunes and others added 5 commits January 28, 2024 11:18
To support adding a persistent link to the sidebar, we need to maintain our own component.

This commit takes the original component from the Starlight codebase and appends our link.
Addresses the point raised in #44
Base automatically changed from starlight-migration to main May 6, 2024 16:11
Copy link

netlify bot commented May 6, 2024

Deploy Preview for openpodcastapi ready!

Name Link
🔨 Latest commit d9c0d1b
🔍 Latest deploy log https://app.netlify.com/sites/openpodcastapi/deploys/66f805913c3347000876258f
😎 Deploy Preview https://deploy-preview-50--openpodcastapi.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@georgkrause
Copy link
Collaborator

@Sporiff Can you rebase this?

@Sporiff
Copy link
Member Author

Sporiff commented Sep 28, 2024

@Sporiff Can you rebase this?

@georgkrause done

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.

4 participants