search

airr-community / airr-standards

AIRR Community Data Standards
https://docs.airr-community.org
Creative Commons Attribution 4.0 International
35 stars 23 forks source link

ADC API v1.2 release branch #612

Closed bcorrie closed 1 year ago

bcorrie commented 2 years ago

Uses spec v1.4.0 tag

bcorrie commented 2 years ago

Closes #607

javh commented 2 years ago

Merge after v1.4 tag is present.

bcorrie commented 1 year ago

@schristley I don't think we want to merge this into master - correct? We want to tag this branch as v1.2.0 but we don't want it merged into master as we don't want the 1.4.1 links in master. I think we want to close this pull request without merging. I have merged master into this branch for the tag.

bcorrie commented 1 year ago

I have confirmed using the swagger editor that both API specs have no errors and work as expected.

schristley commented 1 year ago

@schristley I don't think we want to merge this into master - correct? We want to tag this branch as v1.2.0 but we don't want it merged into master as we don't want the 1.4.1 links in master. I think we want to close this pull request without merging. I have merged master into this branch for the tag.

I think you want to merge to master and tag on master. Maybe @javh has an opinion on this. I guess it all depends when master is going to be switched to V2 development, presumably at that point a V1 branch will need to be created for any V1 fixes and such.

bcorrie commented 1 year ago

@javh any strong opinions? As @schristley says, in a way it does make sense to merge to master, then tag ADC v1.2, then move to master development as per v2.0 work.

javh commented 1 year ago

Not sure I follow. I haven't been keeping track of what's going on with the adc-api branches. The only tags we have on master are for the standards (eg, v1.3, v1.4, etc). Are you wanting to add extra versions for the ADC API to the master branch?

Master is now whatever comes after v1.4.1, whether that's v2 or v1.4.2. (I hope it's v2, but time will tell). I wasn't intending to preemptively create a v1 maintenance branch. If we need one, we can create it when necessary.

bcorrie commented 1 year ago

We need to tag a version of the repo with the ADC API specs using the v1.4.1 branch of the spec. We needed the spec tagged to do this of course. This is done on the adc-api-v1.2 branch. So now we need a tagged version for the v1.2 spec. So I can either:

Regardless we have to merge some of the content here with the master branch - as the final ADC API v1.2 changes are in this branch and we want them to be on master moving forward. So it is really only the question of where we tag the release - off of master or this branch. @schristley suggested off of master might be better - I think suggesting all tagged releases should be off of master?

schristley commented 1 year ago

Not sure I follow. I haven't been keeping track of what's going on with the adc-api branches. The only tags we have on master are for the standards (eg, v1.3, v1.4, etc). Are you wanting to add extra versions for the ADC API to the master branch?

Yes, most likely. This is the first release where we've been discussing tagging the ADC API separately from the AIRR Schema. Previously one used the standards v1.3.1 tag if you want v1.0 of the ADC API. Allowing for extra ADC API tags provides flexibility.

An alternative is that the ADC API release is a standards PATCH release, so e.g. standards v1.4.2 is used for V1.2 of the ADC API. This is problematic because the API spec is self-referential, i.e. the spec directly references the schema version. Plus it's less flexible, if we need to make an API patch release with a bug fix that implies a new standards version.

Regardless, the current versioning and release process documented in CONTRIBUTING doesn't discuss the mechanics for releasing ADC API versions, so that's the main point. What do we want that process to be?

javh commented 1 year ago

Hrm. It's not clear to me what the right approach is. Maybe we can sort this out on a call?

If the ADC API is an application, like the R and python libraries, then ADC API updates being a patch release makes sense, but not with its own version number. Meaning, there is no ADC API v1.2 - there is just the standards release v1.4.1 (well, v1.4.2 because we didn't sort this out before release).

I guess I would be incline to leave the ADC API as a branch for now and figure out a proper solution for v2. Standards releases have been on ~2 year cycle and that could be significantly longer after v2.1, so we need to think about what that means for ADC API.

bcorrie commented 1 year ago

The main problem is that in our past protocol the v1.3.1 release tag had the ADC API specs with the correct AIRR version tag in those ADC API specs.

See: https://github.com/airr-community/airr-standards/blob/725baa9cacf0db009e1bfbb276837c9a05d1f965/specs/adc-api.yaml#L76

The AIRR v1.3.1 tag has the ADC API pointing to a v1.3 AIRR spec (not the correct one it turns out).

In this release we do not have this - the 1.4.1 tag has the master link in the ADC API spec:

See: https://github.com/airr-community/airr-standards/blob/b12fa27e590b762871266bd1fb9762848e236051/specs/adc-api.yaml#L76

The adc-api-v1.2 branch has this fixed.

See: https://github.com/airr-community/airr-standards/blob/e1a74845c114892483837e3a1cc5202bc26402ff/specs/adc-api.yaml#L76

Since it is self referential as @schristley says, it seems like the correct solution is for an AIRR release tag to always contain the ADC specs with the correct references to that version of the AIRR Spec. It seems to me that anything else is probably just wrong 8-)

bcorrie commented 1 year ago

For me, I think this can wait until the Sep 12 meeting if need be. We actually use the AIRR Spec rather than the ADC API spec to define what is in our repositories, so we ultimately need this resolved but it isn't urgent in that this is just a tag for clarity as to what exactly the ADC API v1.2 is.

So we are using the v1.4.1 airr-schema.yaml file, we don't use the adc-api.yaml file. @schristley I think is different in this regard, as his services use the ADC API files directly.

javh commented 1 year ago

Gotcha. We have to do the same self-referential thing with the immcantation docker container (tag version, then build off that tag). It just means things are technically broken for a couple hours.

If the only change is in the ADC API specs/docs and we can restraint from committing to master in the meantime, we can just move the v1.4.1 tag. It's a mild annoyance to fix in ReadTheDocs, but not that big of a deal.

bcorrie commented 1 year ago

Fixes included in #643 closing without merge.