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

docs: Expand versioning suggestions #799

Merged
merged 1 commit into from
May 3, 2021

Conversation

jskeet
Copy link
Contributor

@jskeet jskeet commented Mar 31, 2021

Addresses #778

Signed-off-by: Jon Skeet jonskeet@google.com

@jskeet
Copy link
Contributor Author

jskeet commented Mar 31, 2021

Note: I'm definitely not expecting this to just be approved and merged. I'm very happy to take either specific suggestions, or a more general change in direction ("give concrete examples" or "don't ramble on for so long") to work on. I just wanted to make some kind of progress :)

primer.md Outdated
## Versioning of CloudEvents

For many CloudEvents, the schema of the data within the event might
change over time. The CloudEvents specification does mandate any
Copy link

Choose a reason for hiding this comment

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

did you mean to say "...does not mandate..."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely - well spotted. Not sure how I fouled up that C&P. Will fix and repush.

Copy link
Member

@erikerikson erikerikson left a comment

Choose a reason for hiding this comment

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

Thanks for these improvements. This guidance is better than what it replaces.

I offer mostly nits and munging (sorry but hope they can help comprhension) but also one point on 269 suggesting a stronger declaration of best practice and personal preference.

For many CloudEvents, the schema of the data within the event might
change over time. The CloudEvents specification does not mandate any
particular pattern to be used, or even the use or consideration of
versioning at all. This decision is up to each event producer.

This comment was marked as resolved.

who have identified a CloudEvent type will generally expect the data
within that type to only change in backwardly-compatible ways,
unless clearly indicated otherwise. The precise meaning of
"backwardly-compatible" will vary by data content type.

This comment was marked as resolved.

primer.md Outdated

When a CloudEvent's data changes in a backwardly-compatible way, the
value of the `type` attribute should generally stay the same. The
value of the `dataschema` attribute may change, or it may stay the
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest s/may change, or/should change, though/. The implication otherwise is that assistive tooling (e.g. IDE plugins) or event consumers might have to poll for schema content changes rather than be able to depend on the unique resource identifier that a URI can provide. I appreciated we are not forcing implementer's hands but the use of semver minor and patch version designators in URIs seems like a good best practice to encourage. As stated, the mays seem equivalent and the implication I take away (when placing a probably undue level of attention here) is that we don't have an opinion. At least I have a strong preference for the former, I can only speak for myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm more on the fence - partly aware that I'm pretty sure our dataschema attributes aren't versioned that way.

I've made it "should" but acknowledged that the alternative is simpler... along with the reasons why it's more consumer-friendly to version.

I deliberately haven't called out semver minor changes here, partly as I can imagine one simple schema for dataschema attributes being a link to a file on GitHub, including the commit. That still has the "cache-busting" property, but doesn't require every schema change to be considered as "minor or patch" based on whether it's just a refactoring/comment change, and doesn't require tagging etc.

As ever, feel free to push back - and thanks for the input.

Copy link
Member

Choose a reason for hiding this comment

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

Should feels like a win and as much as I can reasonably ask. Thank you. I think there's a lot of room for a "latest"/"evergreen" link that one would find and use through the discover service/portal but have thought that a strictly versioned dataschema would be best for each event, if one is even sending the attribute. I have to admit not having thought of using commits in the URI but I now in hindsight it seems indefensible. :D

the value of the `type` attribute should generally change. The event
producer is encouraged to produce both the old event and the new
event for some time (potentially forever) in order to avoid
disrupting consumers.

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved things around quite a bit now... as you suggested, it was a matter of "top-level, then more detail" but it's now reorganized to treat each attribute separately. That's also involved moving the general encouragement to earlier on, which I'm generally pleased about. See what you think.

primer.md Outdated
may choose any versioning scheme they wish, such as a version number
(`v1`, `v2`) or a date (`2018-01-01`) as part of the value. They may
also use the `type` attribute to convey that a particular version is
not yet stable, and may take breaking change.
Copy link
Member

Choose a reason for hiding this comment

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

nit: this too seems better grouped with the section addressing type. I suspect you're trying to get into the meat rather than hide it behind too much detail. I might suggest that the top level description paragraph ending on line 251 could benefit from the content starting at 267 and having that stated up front would allow for a slightly deeper indulgence in the full details of best practice for both type and dataschema separately.

primer.md Outdated
Event producers are encouraged to consider versioning from the
outset, before first declaring a particular CloudEvent to be
"stable". Documentation of the chosen versioning scheme, including
the rationale behind it, is likely to be appreciated by consumers.
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@erikerikson
Copy link
Member

Thank you for the updates, it feels a lot easier to absorb now. LGTM

disrupting consumers.

When considering the value of the `type` attribute, event producers
may choose any versioning scheme they wish, such as a version number
Copy link
Collaborator

Choose a reason for hiding this comment

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

overall this seems ok, but it does drop the idea of no versioning scheme at all for "type". Was that intentional? The use of the phrase "may choose any" kind of implies they need to choose one. The old text implied it was optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that wasn't intentional - although I do want to encourage it.

How would this be:

When considering the value of the type attribute, event producers
may choose any versioning scheme they wish, such as a version number
(v1, v2) or a date (2018-01-01) as part of the value. Alternatively, they
may choose not to include a version in the type value at all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure that works - thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in an extra commit. Thanks for the careful review :)

@jskeet
Copy link
Contributor Author

jskeet commented Apr 16, 2021

Now squashed into a single commit. I suspect (contrary to my expectations yesterday) that this can be merged as-is rather than waiting for #810 (the master branch equivalent) to be merged.

Addresses cloudevents#778

Signed-off-by: Jon Skeet <jonskeet@google.com>
@duglin
Copy link
Collaborator

duglin commented May 3, 2021

Approved on the 4/29 call

@duglin duglin merged commit 06e1536 into cloudevents:v1.0.1 May 3, 2021
@jskeet jskeet deleted the primer-version branch November 23, 2021 08:58
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