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

Define rule for breaking and non-breaking changes #96

Closed
ThomDietrich opened this issue May 3, 2018 · 28 comments
Closed

Define rule for breaking and non-breaking changes #96

ThomDietrich opened this issue May 3, 2018 · 28 comments
Labels
Status: Discussion RFC Type: Documentation Issue/PR is solely related to documentation and doesn't change technical details of the convention.

Comments

@ThomDietrich
Copy link
Collaborator

ThomDietrich commented May 3, 2018

SemVer clarifies how to increment a software version:

Given a version number MAJOR.MINOR.PATCH, increment the:

  1. MAJOR version when you make incompatible API changes,
  2. MINOR version when you add functionality in a backwards-compatible manner, and
  3. PATCH version when you make backwards-compatible bug fixes.

The rules are oriented at an application compliant with an API definition.
Here is the issue: Homie IS the definition.

Example: Consider the addition of a datatype #72.

  • From an API point of view, this is a simple addition and doesn't alter or deprecate existing definitions
  • From a client point of view, this addition is irrelevant and not critical because it either doesn't know it (older version) or uses it (new version)
  • From a controller point of view, this is a breaking change as new unexpected interface behaviour is introduced

We need to clarify which viewpoint we want to apply. Please add your opinion.

@timpur
Copy link
Contributor

timpur commented May 3, 2018

For me i think the client is the main concern here, its the main point of homie, with out the client there no point of a controller. Further to that point a controller will most likely be a computer / server which has magnitudes of more resources than an embedded IOT device, thus can handle the multiple version support issues.

A simple way for the controller to do this is for it to only support up to a certain version. When a new minor version comes out, the controller will have to implement the new additions and bump up its supported version number and thus older clients and new clients are all supported. This is something that i implemented in my controller for HA. To me it was just annoying side effect, that some one has to suffer with, other wise will be going through MAJOR version like no tomorrow.

The controller should only need to support the one 'newer' version which should support the older versions for that MAJOR other wise this is a MAJOR change

Just my thoughts....

@ThomDietrich ThomDietrich added Status: Discussion RFC Type: Documentation Issue/PR is solely related to documentation and doesn't change technical details of the convention. labels May 4, 2018
@davidgraeff
Copy link
Member

davidgraeff commented May 4, 2018

@timpur That's a super narrow-minded viewpoint. Controllers can be expected to evolve in a much slower pace than the client versions, because they are probably part of something bigger (ESH, HA) and every change usually needs approval and review of different parties, which can take months.

A controller implemented for a major version, should work for the entire lifetime of that major version. Guidelines/Definitions defined in #51 will help to make sure that happens. An implementation guideline could be for example that a new enum value (may it be in $type,$unit, etc) will cause the controller to log the node/property in question and ignore it.

That's what I'm doing in ESH btw. A ready state not defined in the current spec is mapped to "unknown" and the device it set to offline. A $type not part of the current defined types maps to "unknown" and the property is ignored.

@timpur
Copy link
Contributor

timpur commented May 4, 2018

That would be ideal, but as you said before something as simple as multiple IP support will require a major update ..... That's kinda sad ... :/

@davidgraeff
Copy link
Member

SemVer should be fully applied. I vote for the last option and to specify that SemVer is used somewhere in the convention.

@timpur
Copy link
Contributor

timpur commented May 9, 2018

The only disadvantage to doing it like how you say David, means we'll have to save up breaking changes to a point where they make sense to release a new version. With means waiting around alot ...

@timpur
Copy link
Contributor

timpur commented May 9, 2018

Been thinking about this more and more, and it just sounds observed (^).

Lets take MQTT as an example. MQTT has v3.1.0 and v3.1.1, and each version has a spec. When v3.1.1 came out it meant that servers now supporting the new v3.1.1 could support v3.1.0 clients and also v3.1.1. This make sense, and we'd expect this, but @davidgraeff your saying that a v3.1.1 client should also work on a v3.1.0 server, which it doesnt, i tested it .... and we would never expect this behaviour. Does this mean they broke Sem Ver.... dont think so?

How can a server ever support an future client version?? This has always been the case with the server/client model. The server always deals with the shit, and the client get right of way .....

I dont think it should be any different here.

@timpur
Copy link
Contributor

timpur commented May 9, 2018

If david's model was right then MQTT version 3.1.1 should of been v4.0 as it broke older servers trying to support newer clients, thus 4.0.0, and I could imagine if this was used everywhere around the world, every client/server library would be at v1000000000.0.0

:P , or I'm seriously missing something ?

@davidgraeff
Copy link
Member

Yup you are missing something. Mqtt 3.3.1 is actually mqtt 4. That's the reason why the new version is called mqtt 5. They just fucked up with version numbers.

@timpur
Copy link
Contributor

timpur commented May 10, 2018

Links?

Still this means that any new feature for Homie and something like REST API would actually be a MAJOR change no minor. If i want to add a new search feature like a new optionional param to a search API, your saying this is a MAJOR change. Since clients using this new feature won't work on older APIs and will break. I don't think this is the case, API's receives new features all the time without bumping MAJOR, only when they break backwards support with the client....

If we take your opinion, then every feature will be breaking by definition. I see this slowing Homie's progress alot....

@davidgraeff
Copy link
Member

davidgraeff commented May 10, 2018

Every MODIFICATION of an existing feature causes a major update yes. additions not. Enumeration (consisting of strings, not bit encoded) value removals also not.

Mqtt is another case, they have a single byte indicating the wire protocol. And 3.1.1. was wire protocol version 4 (the wire protocol was changed enough to justify a new version).

Rest APIs are usually Versionen by a single value not semver. Like v1, v2 and each version has a dedicated entpoint.

@timpur
Copy link
Contributor

timpur commented May 10, 2018

So what's your saying is if I add an additional optional parameter to an rest interface I've make a breaking change and should update the major. Since newer clients trying to use this additinal param on a older rest interface would break. That sounds observed to me.

Even if this is the 'correct' way, this would have serious cost on progress of homie and I don't think it's worth that cost. Any progres and new feature a would be halted to major version release.

Which means we're stuck with 3.0 for a very long time till we get 4.0 ready. 4.0 would take a very long time since we only get one shot at realising new features and community project are teriable at agreeing (eg this).

Honestly in my humble opinion, this would kill homie (were small and already have limited progress...). Id probes just fork..., I can see where this would take us. Way to many majors with no implementations and no interest when a Major Changes is need just to support multiple IP address.

I'm fine with the idea of servers needing update to support newer clients, IMHO this is expected from a client server model. Rather that then see homie slowly fade away.

My final words on the matter, you guys make the final decision for this ticket.

@timpur
Copy link
Contributor

timpur commented May 10, 2018

@ThomDietrich

@davidgraeff
Copy link
Member

I have said:

Every MODIFICATION of an existing feature causes a major update yes. additions not. Enumeration (consisting of strings, not bit encoded) value removals also not.

And you concluded:

So what's your saying is if I add an additional optional parameter to an rest interface .... and should update the major

How does this come together? Would be very kind if you read the entire post, especially if it is that short.

You are also assuming this is a server/client protocol specification. It is not. Each "member" of a MQTT network is a client. The homie device as well as the homie controller. They both need to speak the same language. Rules of SemVer apply for both. Except if you say, that a device<-->controller should only be able to communicate when they have the exact same version.

@timpur
Copy link
Contributor

timpur commented May 10, 2018

I read you post entirely, your fist statement is confusing, removal of attributives will break, older controllers (servers) expecting this value ....

The rest example i used is similar to your #95 (comment), newer clients trying to call a rest interface with the new attribute will break on older rest server versions.

Your absolutely right about Members of MQTT being clients, but i never talked about MQTT members. Homie uses a client / controller (discovery server) model on top of MQTT, where MQTT is just the communication method like TCP.

Also i never said the exact same version, if you were following, im saying that a controller can support clients up to it's major and minor, but no future minor, as this is a future version. Can you really expect a server to support future clients, id love it if you could, but whats the cost????

Have a think about all the scenarios that will break compatibility in context of client and server, please define all the rules, and propose it for everyone to read. Whats the cost of your proposal and what does it mean to Homie, pros and cons. Ill do the same if you'd like?

@davidgraeff
Copy link
Member

davidgraeff commented May 11, 2018

... im saying that a controller can support clients up to it's major and minor, but no future minor

You are making the minor version as important as the major version with this statement ^^. For a controller implementation it doesn't change much if I restrict from "3.0<=x<=3.1" or from "3.0<=x<4.0".

The idea is to not change anything of the core convention that would prevent a device with 3.x to communicate with a controller supporting 3.x (following your versioning it would be: a device with 3.1.x to speak to a controller 3.1.x). A major version need to stay and be supported for about half a year minimum. I mean, you want this convention to be accepted by different parties. Make it stable.

And that's why I suggested to remove all topics from the convention that are still part of discussions like $stats and arrays. Those can be part of an "Experimental" section or so. But they will change in the future and shouldn't require the maintainer to bump the major version.

Major version bumps

Breaking changes are:

  • Changing the semantic of an existing field. For example $localip would contain an IPv6 address set.
  • Removing a field that is not self-containing like removing $format, which is coupled to $type.
  • Changing the topic layout (e.g. changing how arrays are organized)

Minor changes are:

  • Adding an enum value
  • Removing an enum value (e.g. removing the $type "float". Nothing breaks on the controller side, and the device is just sending invalid/ignored Properties, which is fine).
  • Adding a node, a property, an attribute
  • Removing a self-contained property (like a $stat/xxx).

Super minor changes are:

  • Removing an optional node/property/attribute
  • Changing the wording, give a more specific, maybe a more limited idea how a node/property/attribute works.

Visible document version

The full version should always be set in the visible document. E.g. "3.1.2". A PR should always also adapt the version (mostly the Revision number) and should be declined if it doesn't. Major/Minor version number changes should be tagged.

@davidgraeff
Copy link
Member

@ThomDietrich, @timpur Can this now be closed with the decision to use semantic versioning?

@ThomDietrich
Copy link
Collaborator Author

The "decision" was made a long time ago. After all the discussion in various places I believe we need to add a clear set of rules (e.g. based on my "receiving partner" argument from #108) as PR in CONTRIBUTING.md. Would you agree?

@timpur
Copy link
Contributor

timpur commented Oct 29, 2018

This is one of those things I think we should hash out on slack and post our outcomes here.

I am worried that even a simple addition to the convention actually results in a major version change. As you have stated david, should the Stateful attribute change actually of been a major version change ? Discovery services are now expecting a new attribute that doesn't exist on 3.0.0 but does on 3.0.1?? Is this not breaking change ? So we should really be on 4.0

This is why I feel this is a bit over the top. We need a solution where every addition isn't considered a breaking change?

If you want to take this route then the only way I see this working is we bunch some additions into a single major release. But this will slow down change ....

@lorenwest
Copy link
Member

@timpur - This is a good reason to keep these discussions public, because your worries are based in misunderstanding semver.

Semver doesn't try to define breaking - only backwardly compatible. The stateful attribute was carefully designed to be backwardly compatible, so no way would it qualify as a major. It wasn't a fix to something in the spec that was broken (such as the discussion about topic ordering), so it isn't a patch.

It can only be a minor, and minor version changes aren't considered bad. In fact, they're considered good because it'a new feature that someone spent time making backwardly compatible.

Once you've worked with semver a while, your fear that people will mis-categorize every change as a major will pass.

@timpur
Copy link
Contributor

timpur commented Oct 30, 2018

@lorenwest
Firstly I never said make the convo private. It should always be public, I just mention that because I think IM could make resolving these issues quicker.

Secondly I never said it was bad. I worry because majors mean you've broken backwards comparability (aka a breaking change ????) Thus can a Homie discover service be expected to work for multiple majors ? I'm assuming not so a discover service will only ever really work for one major. It could work with more, but now your asking alot from these discovery services (W3 allover again). Thus we want to limit the times we make breaking changes so that the entire homie stack for the user doesn't need rework and updating. ???

Finally yes I was wrong about the stateful attribute example, but say we did add an attribute that was required. That would be a breaking change. So that's a major version bump. Something simple like this means that now you have to pick between updating all your devices since you can only use one major with in a network ( for a single discover service ) or live with being out of date ??

I think these are things we need to think about. How do we go about improving the standard while not make it fustrating to the users.

If you guys want to stick to letter of semantic Versioning for Homie then I think the only way is to release a batch of breaking changes?? Is this how most libs do it ?

@davidgraeff
Copy link
Member

Personally I find myself agreeing with @lorenwest. Semver will make the minor version bumped more often, but that is not a bad thing. It is a little harder for marketing Homie and the stability of Homie because not all people are familiar with semver. But I have a plan for the website to take this fear of different minor versions away.

Let's summarize:

  • A homie controller can expect to communicate with all devices with the same major version.
  • A homie device can expect to be understood by a controller with the same major version.
    • A higher minor version is ok -> The controller will not understand newer additions.
    • A lower minor version is ok -> The controller can understand way more than the device provided but can work with what is there.

On the implementation website I will list all Controller and Device implementations with "3.x" and "2.x" which expresses exactly this. The minor version is only of lower relevance for a communication to be successful.

@timpur
Copy link
Contributor

timpur commented Oct 30, 2018

@davidgraeff I can agree to that
Think I've mentioned my worries. I'm in for semiver just worried it might show down change in the long run.

I approve of those conditions

@lorenwest
Copy link
Member

lorenwest commented Oct 30, 2018

@timpur - Your fears about change having negative effect are well founded. The way this works itself out in a semver world is we agonize about introducing features that aren't backwardly compatible. It quickly squashes discussions about incompatible features, and forces us to go the extra mile to guarantee compatibility within a major release.

In the end, this is very good for instilling trust and stability.

@lorenwest
Copy link
Member

@davidgraeff - On the topic of semver making it hard to market the spec, my experience is that people that don't understand semver won't even notice you're using it. They simply enjoy the benefits.

The only case that not understanding semver comes into the picture is when someone wants to introduce a change, and doesn't understand the importance of compatibility. It's a short discussion.

@timpur
Copy link
Contributor

timpur commented Nov 4, 2018

@lorenwest , i am fully aware of semver, but my point still stands, following @davidgraeff points, this means we will have to try reduce the frequency of Major releases otherwise no one will keep up to date. Updating an entire network to a new major every month is something no one wants to do.

So what is the plan here, when do we decide to release a Major ??

@davidgraeff
Copy link
Member

@timpur There is not much left to discuss. I guess 4.x will be our last "breaking" release. We should maybe do that before end of the year

@lorenwest
Copy link
Member

@timpur - You're absolutely right. Updating majors too often is a sign of poor planning, and it will give the API a bad name. Best to spend extra effort making changes backward compatible, and save breaking changes for major releases - not too often - maybe once a year.

@timpur
Copy link
Contributor

timpur commented Nov 5, 2018

I like this for every major:
image
Thanks @davidgraeff

@timpur timpur closed this as completed Nov 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Discussion RFC Type: Documentation Issue/PR is solely related to documentation and doesn't change technical details of the convention.
Projects
None yet
Development

No branches or pull requests

4 participants