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

json-output: Release format version 1.0 #29502

Merged
merged 1 commit into from
Sep 9, 2021

Conversation

alisdair
Copy link
Contributor

@alisdair alisdair commented Sep 1, 2021

Update all stable JSON format version numbers to "1.0", and explain what this means going forward in the public documentation. There are no other changes to the JSON format in this PR. This change is targeting a 1.1.0 release.

@alisdair alisdair requested a review from a team September 1, 2021 18:01
@alisdair alisdair self-assigned this Sep 1, 2021
Copy link
Contributor

@apparentlymart apparentlymart 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 updating this!

I left some subjective bits inline, which I'm happy for you to ignore if you disagree.

@laurapacilio might have some less wishy-washy feedback. 😀

Comment on lines 25 to 27
- Any backwards incompatible changes will result in a major version increment,
e.g. `"2.0"`. Consumers of this format should reject unknown major format
versions.
Copy link
Contributor

Choose a reason for hiding this comment

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

I dunno if it necessarily needs to be called out here, but a literal read of this makes it seem in conflict with the Terraform v1.0 Compatibility Promises just because it doesn't say anything about what circumstances we might allow such a change.

Perhaps a reasonable compromise here would be to just add an additional sentence here which connects the two, to make it clear that we're not intending to make an exception here:

  • Any backwards incompatible changes will result in a major version increment, e.g. "2.0". Consumers of this format should reject unknown major format versions. We will introduce new major versions only within the bounds of the Terraform 1.0 Compatibility Promises.

(My intended implication of this reference, then, would be: a major version change to these formats can come only either as part of a future Terraform v2.0 or in response to some explicit opt-in which retains the 1.x format as the default result.)

Comment on lines 22 to 27
- Backwards compatible changes or additions will be made along with a minor
version increment, e.g. `"1.1"`. Consumers of this format should ignore any
unknown keys to remain forwards compatible.
- Any backwards incompatible changes will result in a major version increment,
e.g. `"2.0"`. Consumers of this format should reject unknown major format
versions.
Copy link
Contributor

Choose a reason for hiding this comment

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

We typically try to avoid using the passive voice in our docs, but I know there's some tension in that rule when we're writing "specification-like" documentation, because technical specifications often do use the passive voice and thus we're accustomed to sentences structured like this in that context.

I did take an attempt at active voice alternatives of your two list items here, though I'm presenting them only for consideration and not asserting that they are necessarily better, because I remain kinda ambivalent about whether this is an appropriate writing style for specification-like content:

  • We will increment the minor version, e.g. "1.1", for backward-compatible changes or additions. Ignore any object properties with unrecognized names to remain forward-compatible with future minor versions.
  • We will increment the major version, e.g. "2.0", for changes that are not backward-compatible. Reject any input which reports an unsupported major version.

(while editing this I also swapped "unknown keys" for "object properties with unrecognized names", because "properties" is the term commonly used in JavaScript. Sadly the JSON RFC just refers to these as "name/value pairs", which I always find clumsy to use in prose, so I typically write "property names" in an attempt to meld those two sets of terminology together. Again, I'm just presenting this for consideration, not asserting that it's the "right answer".)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on the active voice! :-) Thank you, Martin. @alisdair I'd love to see a revised version of this with the active voice. Then I can take a final pass for flow, etc.

Copy link
Contributor

@laurapacilio laurapacilio left a comment

Choose a reason for hiding this comment

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

I agree with Martin that we want to try to use active voice here. It's just easier to read and often more concise. I think unless we're literally writing a requirements doc and have to say "X thing shall [description here" we want to go with active. Thank you for tagging me @apparentlymart and thank you @alisdair for doing this!

Comment on lines 22 to 27
- Backwards compatible changes or additions will be made along with a minor
version increment, e.g. `"1.1"`. Consumers of this format should ignore any
unknown keys to remain forwards compatible.
- Any backwards incompatible changes will result in a major version increment,
e.g. `"2.0"`. Consumers of this format should reject unknown major format
versions.
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on the active voice! :-) Thank you, Martin. @alisdair I'd love to see a revised version of this with the active voice. Then I can take a final pass for flow, etc.

@alisdair alisdair force-pushed the alisdair/json-format-version branch from 4b962b4 to b60f201 Compare September 1, 2021 19:15
@alisdair
Copy link
Contributor Author

alisdair commented Sep 1, 2021

Thanks for the suggestions! I've incorporated them verbatim.

Copy link
Contributor

@laurapacilio laurapacilio left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM!

@hashicorp-cla
Copy link

hashicorp-cla commented Sep 9, 2021

CLA assistant check
All committers have signed the CLA.

@alisdair
Copy link
Contributor Author

alisdair commented Sep 9, 2021

Reverting this as it was not ready to be merged.

@alisdair alisdair deleted the alisdair/json-format-version branch September 9, 2021 15:45
@github-actions
Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants