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

TRG API Versioning #526

Closed
wants to merge 6 commits into from
Closed

Conversation

tobzahn
Copy link

@tobzahn tobzahn commented Dec 4, 2023

This TRG Draft adds guidelines for API versioning

The goal is to have a common way to version APIs by putting the API version into the URL of the API
This proposal has been discussed with several stakeholders and is therefore now proposed as a TRG Draft

@maximilianong
Copy link
Contributor

@tobzahn thanks for providing this TRG.
I think @SebastianBezold and @AngelikaWittek are better for review than I am :)
I assigned them as reviewers.

Copy link
Contributor

@SebastianBezold SebastianBezold left a comment

Choose a reason for hiding this comment

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

Hi @tobzahn,

I think overall this TRG is fine. I would just fix all the bold instead of capital occurrences.
What do you think about the proposal to create a dedicates TRG group APIs.
Like "TRG 8 APIs". This could host:

  • TRG 8.1 API versioning
  • TRG 8.2 Naming guidelines for APIs
  • TRG 8.3 Deprecating APIs
  • TRG 8.4 publishing the openAPI.yaml

Not saying, that you need to provide all these, but I think the API topic definitely has the potential to grow. Like also mentioned: From my personal experience having these "small" and focused TRGs makes it easier to describe them, easier to understand them and easier to verify them in a quality gate or PR review

docs/release/trg-0/trg-0.md Outdated Show resolved Hide resolved

For publicly available APIs a versioning mechanism is required, so that all APIs are versioned the same way.

Note that for APIs accessible only via EDC, the API-version is included in the EDC asset and not the URL. This is described in CX-0018.
Copy link
Contributor

Choose a reason for hiding this comment

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

CX-0018 should be linked to the standard document

Copy link
Author

Choose a reason for hiding this comment

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

@SebastianBezold I can do that - to what URL should I link to? Is there a best practice?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I thought there is a public doc already behind the standard ID, since i saw that in some of the KITs.
Example taken from Adoption view of RuL
CX - 0059 Triangle Behavioral Twin Endurance Predictor:

I think some of the Links I saw are pointing to the overall page containing all the standards, but that was an exception, because AFAIK, the standard is implemented together with the KIT and not yet officially released in public

docs/release/trg-0/TRG 6.02 API Versioning.md Outdated Show resolved Hide resolved

For the version in the URL, semantic versioning MUST be used.

Old endpoints (that have been offered bevor) and are not supported anymore MUST respond with an appropriate HTTP 30x status code and the new location (URL).
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a dedicated "Deprecate APIs TRGs". From my experience, it is easier to have more TRGs, that are then more precise and easier to verify and comply with fully.

docs/release/trg-0/TRG 6.02 API Versioning.md Outdated Show resolved Hide resolved

For the version in the URL, semantic versioning **must** be used.

Old endpoints (that have been offered bevor) and are not supported anymore **must** respond with an appropriate HTTP 30x status code and the new location (URL).
Copy link
Author

Choose a reason for hiding this comment

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

@SebastianBezold You had suggested to open another TRG for this. I would rather leave it here, as it is in direct relation with API versioning. We could instead create a new section (with a headline like 'old APIs') so that it can be referenced more clearly. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally would still prefer a dedicated TRG. We could still add a small section in this one and additionally link to the other TRG. Like I mentioned, i think this makes it easier to maintian and also easier to understand and follow.

But as also said, this is my personal opinion and we should discuss this with more people, since if this will be released as TRG, everyone has to comply to it.
We could of course also extract it to a dedicated TRG later on

SebastianBezold
SebastianBezold previously approved these changes Dec 7, 2023
Copy link
Contributor

@SebastianBezold SebastianBezold left a comment

Choose a reason for hiding this comment

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

Definitely fine for a draft now. Once merged, we can announce it via mailing list to open it to broader discussion

@tobzahn
Copy link
Author

tobzahn commented Dec 7, 2023

@AngelikaWittek can you please review as well? I would appreciate if we can get it merged and discuss it on the mailing list then, as suggested by @SebastianBezold

@evegufy
Copy link
Contributor

evegufy commented Dec 11, 2023

@tobzahn @SebastianBezold I think this PR should be announced to the mailing list before being merged, so that everybody has the option to opt in on the discussion for a certain amount of time. Once the PR is merged as Draft TRG, it's more difficult to have a discussion on it and to possibly still do changes on it.

@stefan-ettl
Copy link

Hi @tobzahn
I see an issue by defining such a dop-down rule. Asking for a general versioning of APIs in Tractus-X is ok but to prescribe how to do it is an implementation detail that should be decided by each project.
Please be aware, there are different dependencies, like Protocols that might run into conflicts and are not under our full control (i.e. DSP and IATP). Especially the redirect rule could cause issues here...

@jimmarino
Copy link

jimmarino commented Dec 11, 2023

Hi @tobzahn I see an issue by defining such a dop-down rule. Asking for a general versioning of APIs in Tractus-X is ok but to prescribe how to do it is an implementation detail that should be decided by each project. Please be aware, there are different dependencies, like Protocols that might run into conflicts and are not under our full control (i.e. DSP and IATP). Especially the redirect rule could cause issues here...

To reiterate what @stefan-ettl said, the rules specified here will come into conflict with how other standards (e.g., DSP) and required upstream projects that Tractus-X has no control over (e.g., EDC) handle versioning. Suggestions and recommendations are fine, but placing requirements on other projects and standards is not practical. Generally, those projects have good reasons for adopting the schemes they do, and it would be better to defer to them.

@tobzahn
Copy link
Author

tobzahn commented Dec 12, 2023 via email

@jimmarino
Copy link

jimmarino commented Dec 12, 2023

Hello @ETTL Stefan, @.>, Hello @jim @.>, Thanks for your reply. When writing the TRG I had things like the portal in mind, but not DSP and IATP. Do you have an idea how this could be better differentiated?

How about working with Portal and specific teams directly to have them decide if it fits their use cases and incorporate the approach directly using whatever testing and verification tools they have adopted?

Included feedback from call and discussion on mailinglist
@carslen carslen added the stale Items without activities label Mar 11, 2024
@danielmiehle
Copy link
Contributor

@tobzahn are you still working on this PR or can we close it?

fyi @giterrific

@arnoweiss arnoweiss closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Items without activities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants