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

Public Review: Breaking Changes #10

Merged
merged 8 commits into from Jul 12, 2023
Merged

Conversation

jeremythuff
Copy link
Contributor

No description provided.

craigmcnally
craigmcnally previously approved these changes May 3, 2023
@craigmcnally craigmcnally dismissed their stale review May 3, 2023 17:09

Misunderstanding - I was approving the change specifying the RFC phase

@jeremythuff
Copy link
Contributor Author

jeremythuff commented May 30, 2023

The following comments on this RFC come from @MikeTaylor, who shared them on a Slack thread in response to an announcement on the Stripes channel about this RFC being in the public review stage:

  • Typo “This RFS seeks”
  • Typo “and does address”
  • Pet nit-pick: “Search query” is redundant. A query is a static thing, such as author=kernighan. A search is the process of executing that query. A search query means, if anything, a query that is used in a search: but all queries are used in searches.
  • “our messaging implementation does not version messages” feels like a pretty freakin’ huge hole.
  • “Breaking Change: A change that requires a Major Version Change as per Semantic Versioning’s requirements.” This seems circular to me, SemVer says that a new major version-number is required for a breaking change, so it’s hardly helpful to define a breaking change as one that needs a new major version.
  • “Data Model Change: A change in implementation that will require the addition, modification or removal of tables, columns or JSON structures in the storage layer.” That seems wrong to me. The storage layer could change in profound ways (e.g. switch to Oracle) with no API-visible changes at all. I think a Data Model Change — for useful purposes — means a change in the data model used by the WSAPI and defined by the module’s interface (JSON Schemas, etc.)
    (Here I am using “interface” in the sense that the present document defines, not in the sense that Okapi module descriptors provide and require interfaces.) (edited)
  • I don’t understand the difference between infrastructural changes and non-infrastructural changes.
  • “We will not address what constitutes as a breaking change in the behavioral level, though this will be an important topic to broach in the future.” This is the third time that the document makes this point — maybe one (or two) too many?
  • Regarding:
    The introduction of an additional version of an already consumed OKAPI interface
    The removal of an an an additional version of an already consumed OKAPI Interface
    It seems to me that these are the wrong way round: the former is breaking and the latter non-breaking. If I am wrong, then the document would benefit from the addition of an explanation.
  • “A change of the minimum version of an OKAPI interface already consumed” or may not be breaking, depending on what it is. If I back down from requiring v4.3 to v4.2, that is non-breaking (since v4.3 still fulfils the requirement).
  • I’m not quite clear on what it means for a change in a UI module to be breaking. Breaking for what? What consumes the module, that could be broken by the change? A platform, I guess? It’s worth being explicit about this.
  • I would like to better understand why “a change in the runtime environment” is a breaking change in a backend module.
  • “The addition of a trailing ‘0’ to an interface version” — what does this refer to? Does it mean promoting the version number of a provided interface from x.y to x.y.0? Is that even possible? The last I knew, interface versions were two-facet.
  • “The addition or removal of an HTTP status code from an existing endpoint”. I don’t think removing a possible HTTP status code is breaking. For example, if my module previous could return HTTP 501 Not Implemented but no longer does, that is not breaking as all pre-existing client code will continue to work.
  • In the “Changes to the data model” section, it’s necessary to treat separately data submitted to the module (POST/PUT) from and data retrieved from the module (GET). For example, in the former case, addition of a mandatory field is breaking; in the latter, it is not.

@jeremythuff jeremythuff merged commit de53f60 into master Jul 12, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants