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: add semantic versioning #28

Merged
merged 5 commits into from
Feb 17, 2023
Merged

feat: add semantic versioning #28

merged 5 commits into from
Feb 17, 2023

Conversation

LakiG
Copy link
Collaborator

@LakiG LakiG commented Feb 9, 2023

Creating a separate PR to amend versioning changes as mentioned in #25 since it applies to v1.0.0 and not only v1.1.0.

  • Add versions field in YAML test files
  • Remove the default version in CLI args. It is a required argument now
  • Convert SemVer into Major API version in client.py

@codecov
Copy link

codecov bot commented Feb 9, 2023

Codecov Report

Base: 99.62% // Head: 99.62% // No change to project coverage 👍

Coverage data is based on head (6f109d2) compared to base (395972f).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev      #28   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files          10       10           
  Lines         527      527           
=======================================
  Hits          525      525           
  Misses          2        2           
Impacted Files Coverage Δ
compliance_suite/constants/constants.py 100.00% <ø> (ø)
compliance_suite/cli.py 94.73% <100.00%> (-0.14%) ⬇️
compliance_suite/functions/client.py 100.00% <100.00%> (ø)
compliance_suite/test_runner.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@LakiG
Copy link
Collaborator Author

LakiG commented Feb 9, 2023

This puts quite some assumptions on how the version looks like. We currently deal with v1.0 and v1.1 in TES, but note that including v is non-standard according to SemVer. It's also possible to have major, minor or patch versions with more than one digit.

I think you should better write/find/import a SemVer parser (that may optionally have a leading v).

Continuing on #25 (comment) here.
I have made a modification to pass proper SemVer format to client, but the client has to anyways convert it into the major API version. The hosted TES server URLs don't support Minor/Patch version currently. Eg. https://csc-tesk-noauth.rahtiapp.fi/v1/service-info.

@LakiG
Copy link
Collaborator Author

LakiG commented Feb 9, 2023

Additionally, unless there is a standard for TES servers to host URLs with API context path containing the SemVer version, we will have to also add a new CLI arg to instruct client whether to use Major or SemVer version every time.

@LakiG LakiG requested a review from uniqueg February 9, 2023 19:52
compliance_suite/cli.py Show resolved Hide resolved
compliance_suite/functions/client.py Outdated Show resolved Hide resolved
tests/template/test_template_schema.json Outdated Show resolved Hide resolved
unittests/test_test_runner.py Outdated Show resolved Hide resolved
@LakiG LakiG requested a review from uniqueg February 12, 2023 23:04
@uniqueg
Copy link
Member

uniqueg commented Feb 14, 2023

This puts quite some assumptions on how the version looks like. We currently deal with v1.0 and v1.1 in TES, but note that including v is non-standard according to SemVer. It's also possible to have major, minor or patch versions with more than one digit.
I think you should better write/find/import a SemVer parser (that may optionally have a leading v).

Continuing on #25 (comment) here. I have made a modification to pass proper SemVer format to client, but the client has to anyways convert it into the major API version. The hosted TES server URLs don't support Minor/Patch version currently. Eg. https://csc-tesk-noauth.rahtiapp.fi/v1/service-info.

Actually, that is not really true. According to the specs, the API is to be hosted at /ga4gh/tes/v1 and previously (but a long time ago) was to be hosted at just /v1. But while the choice to include the major version in the path like that seems to be a sort of convention at least for the GA4GH Cloud APIs, it does not necessarily need to be the case for future versions - which might have the version encoded in some other way or, theoretically, not at all. And of course the same is especially true for other OpenAPI specs. So I think we should rahter find a way not to hardcode any kind of ad hoc rule here that works for one spec now.

What I think the test runner should do is to take the user's input and apply the rules encoded in the servers section of the OpenAPI specs to it in order to find one or multiple possible API locations, then try them out, successively (if there are more than one, for TES there is only a single suffix). For additional flexibility you could also add an empty string suffix as well. That way, if people choose to deploy a TES (or other, in the future) API instance at an off-spec location, then they could still use the runner by providing the entire path to where the API is hosted.

I have done something like that in ohsu-comp-bio/py-tes#46

@LakiG
Copy link
Collaborator Author

LakiG commented Feb 16, 2023

Thats right, the server base path might not necessarily be a "version". We should think of a solution which applies to all OpenAPI specs.
The flexible server URL feature seems great for py-tes which has to connect with the client and fetch a response. For a compliance suite however, it would defeat the purpose of strict checks. Since the server URL is provided by the user in runtime, they are already aware of the working URL and the suite does not need to find a correct URL from the provided value. Although, a TES test server rule to check if the server URL ends with ga4gh/tes/v1/ can be added, but this again is on the stricter side (similar rules can be added for respective OpenAPI specs).

A solution I can think of is to provide the full API URL, including the base path and excluding the endpoints. eg. https://<host>/ga4gh/tes/v1/. This way we can assume that the provided value could be correct and also solve the above issue of version as the base path currently.

compliance_suite/functions/client.py Outdated Show resolved Hide resolved
compliance_suite/functions/client.py Outdated Show resolved Hide resolved
@uniqueg
Copy link
Member

uniqueg commented Feb 17, 2023

LGTM, I think you can merge

@LakiG LakiG merged commit 433db86 into dev Feb 17, 2023
@LakiG LakiG deleted the feature/semver branch February 17, 2023 16:37
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.

2 participants