-
Notifications
You must be signed in to change notification settings - Fork 291
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
GBFS documentation versioning and and feed conformance #188
Conversation
First draft proposal
README.md
Outdated
|
||
### Version Release Cycles | ||
* There is no strict limitation on the frequency of MAJOR releases, but the GBFS community aims to limit the MAJOR releases to 2 or less every 12 months. To limit releases, breaking changes can be batched together. | ||
* MINOR changes may be applied at any time. There is no guideline to limit the number of MINOR changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are minor changes batched too, or does every change that qualifies as a minor change bump the MINOR
version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was intentionally silent so that MINOR changes could be batched or not batched, depending on their timeline. My assumption is that if changes occur soon after each other, we might as well batch. But, with backwards-compatibility issues absent, it doesn't make sense to hold back changes too long for a batched version bump. I'll explicitly state this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would state this explicitly just so there isn't any confusion.
gbfs.md
Outdated
} | ||
] | ||
}, | ||
"fr" : { | ||
"feeds": [ | ||
{ | ||
"name": "system_information", | ||
"url": "https://www.example.com/gbfs/fr/system_information" | ||
"url": "https://www.example.com/gbfs/1.0/fr/system_information" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we going to remain silent on how versioning should be handled in the URL path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think? Should we offer a recommendation? As this is written, it's up to the discretion of the feed producers. The examples offer suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should state that a URL must never change its major version. For example, if a feed doesn't want to include a version in the path, that's fine, but it must not change from v1 to v2 ever.
This actually brings up another question - can a feed change from 1.0 to 1.1? Technically this should not break anything, and it would mean that publishers could have, say, https://www.example.com/gbfs/1/fr/system_information
for their feed that conformed to the 1.X spec. Would this be confusing for consumers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One challenge for dictating versions in URL paths is that it could conflict with existing versioning schemes or numbers used by producers in URL paths. For example, HOPR currently uses integers in the URL path to identify systems - https://gbfs.hopr.city/api/gbfs/6 vs https://gbfs.hopr.city/api/gbfs/8.
So there could be significant backwards compatibility concerns.
If we require the auto-discovery file (see proposal at #189), I'm inclined to let producers manage their own URL paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcn I just pushed a change that responds to what you pointed out. Please let me know what you think…
Thanks @antrim! Looks pretty good, I suggested some changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing after @barbeau means lots of thumbs-up responses from me. Looking good!
In response to review by @barbeau
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review, @barbeau and @heidiguenin. I've modified the proposal according to most of the suggestions. There is one outstanding question around URL paths and versions.
Proposed next steps
- Let's keep this open until Tuesday (UTC) of next week for review and comment.
- And then call a vote on Wednesday (UTC). Assuming there is support for this proposal, I propose that we also call a vote on changes that break backwards-compatibility at the same time, preparing to implement the versioning process.
A question about the transition from our unversioned to versioned world: should we just state that all unversioned feeds should be considered |
README.md
Outdated
@@ -40,6 +40,26 @@ The general outline for changing the spec has 4 steps: | |||
3. Find at least one GBFS producer to implement and test the proposed change. | |||
4. Submit a final request-for-comments on the proposed change to the issue discussion. If no outstanding issues are identified after one week’s time, and there is general agreement that the proposed change is worthwhile and follows the GBFS guiding principles outlined below, the proposal will be officially adopted. | |||
|
|||
## Specification Versioning | |||
To enable the evolution of GBFS, including changes that would otherwise break backwards-compatibility with consuming applications, GBFS documentation is versioned. A git tag in the form of `X.Y` establishes semantic versions. Multiple changes (commits) may be batched into a single new release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that git tags technically have to start with a letter, so the tag will be vX.Y
where the version name itself will be X.Y
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
_GBFS producers_ should provide endpoints that conform to both the current specification long term support (LTS) branch as well as the latest release branch within at least 3 months of a new spec `MAJOR` or `MINOR` version release. See [specification versioning](README.md#specification-versioning) | ||
|
||
_GBFS consumers_ should, at a minumum, support the current LTS branch. It highly recommended that GBFS consumers support later releases. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this "should" or "must?" If a producer does not provide LTS and the latest feed, are there any repercussions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is framed as a "Best Practice" to work within the bounds of the authority of specification authors. This is a recommendation, not a requirement. Making any sort of requirement with repercussions is the domain of cities and other governments and/or bikeshare contract holders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A clarifying question: what does "It highly recommended that GBFS consumers support later releases." mean?
## Files | ||
This specification defines the following files along with their associated content: | ||
|
||
File Name | Required | Defines | ||
--------------------------- | ----------------------- | ---------- | ||
gbfs.json | Optional | Auto-discovery file that links to all of the other files published by the system. This file is optional, but highly recommended. | ||
gbfs_versions.json | Optional | Lists all feed endpoints published according to versions of the GBFS documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused about where this sits. Does this file get published for every version? i.e. does this file appear in 1.0, 1.1 and 1.2 locations? Regardless, I propose we make it required in order to be able to find all of the relevant feeds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file would sit in all the versions.
Proposal: Let's keep this optional for its incorporation into a "1.0" version. Technically, the change is backwards-compatible if we make the file optional.
We could make this required in a future backwards compatibility breaking MAJOR change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make this required in a future backwards compatibility breaking MAJOR change.
Let's update this field to indicate that it will probably become required in a future version so producers are encouraged to start publishing now.
gbfs.md
Outdated
Example: | ||
```json | ||
{ | ||
"last_updated": 1434054678, | ||
"ttl": 3600, | ||
"version":"1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with the rest of the documentation, wherever new JSON fields were added, can we add a space after the :
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Changed.
gbfs.md
Outdated
"ttl": 0, | ||
"version":"1.0", | ||
"data": { | ||
[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be
"versions": [
(you're missing the key name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Fixed.
"version": "2.1" -> "version": "1.0" as per @jcn suggestion Co-Authored-By: Jesse Chan-Norris <[email protected]>
Change example data for gbfs.json to be consistent with a v1.0 example
Voting has closed. Both Lime and Transit voted for the change. No organizations voted against. Current GBFS governing rules say that at least one producer and consumer have to implement the change before it is adopted, I propose that we merge to the master branch (if there are no objections) for these reasons:
Does anyone have objections to merging into the master branch? |
I am in favor of this change and in merging it at the end of the voting period but for the sake of process we should wait for the actual end as per your original call for a vote.
|
Oops. Sorry about that confusion with the dates. Voting is of course open through Friday. |
+1 for Stae |
May I suggest that if this PR is merged, as it seems it will, then the repository is tagged as soon as possible: we should have, I suppose, at least a version 1.0 tag that pinpoints which of the more than 20 commits claiming to be version 1.0 in their version histories is actually version 1.0. :-) Thanks! |
PS: For what it's worth, as an independent consultant working on an MDS/GBFS related project, I'm in favour of the proposed changes. |
+1 from IBI Group. |
+1 |
1 similar comment
+1 |
The change passes! We had 6 votes in favor from consumers or operators and 0 votes against:
Next steps:
|
fixes the merge of PR #188 addes back section "Files"
We'd love to make this an official part of the spec, but first we need to see this change being implemented. Could you comment here if your organization has implemented this? |
Issue #15 contains a discussion thread about versioning GBFS documentation and data. Versioning is needed to enable backwards-compatibility breaking changes.
This PR:
versionCode
concept (integers associated with each named version), but this could be added if requested.Please comment and propose any changes. @MobilityData proposes to bring this to vote in 1 week or less so that GBFS can begin to incorporate necessary changes that break backwards-compatibility.