[mosquitto] version 4.8.1 is breaking

[morremeyer]

Helm chart name

mosquitto

Helm chart version

4.8.1

Container name

eclipse-mosquitto

Container tag

irrelevant

Description

Upgrading from version 4.7.1 to 4.8.1 or 4.8.2 is not possible without updating configuration, making this a breaking change - 4.8.1 should therefore have been released as 5.0.0.

Expected result

Minor version is not including breaking changes.

For breaking changes, the changelog should contain migration instructions.

Helm values to reproduce

auth:
  enabled: true

persistence:
  data:
    enabled: true
    accessMode: ReadWriteOnce
    size: 100Mi

  # This is only enabled for the line in the config file
  # No data is stored inside. Check home-assistant/mosquitto-config instead
  configinc:
    enabled: true

  passwd:
    enabled: true
    type: custom
    readOnly: true
    mountPath: /mosquitto/config/passwd
    subPath: passwd
    volumeSpec:
      configMap:
        name: mosquitto-additional-config

  additional-config:
    enabled: true
    type: custom
    readOnly: true
    mountPath: /mosquitto/configinc/mosquitto-additional.conf
    subPath: mosquitto-additional.conf
    volumeSpec:
      configMap:
        name: mosquitto-additional-config

### Additional Information

I initially assumed this is due to my configuration, but it occurs as soon as `persistence.configinc.enabled` is set to true.

I'll be submitting a bugfix with a sensible default in a moment.

### Repo link

_No response_

[bjw-s]

Upgrading from version 4.7.1 to 4.8.1 or 4.8.2 is not possible without updating configuration, making this a breaking change - 4.8.1 should therefore have been released as 5.0.0.

Good news, turns out the few people that are maintaining these charts are only human after all!. We do our very best to make sure that we adhere to semver, but every now and then something might slip through the cracks. You are correct that this could have been a breaking change. The chart API hasn't really changed though, only default values were removed. That is always a bit of a grey area, and in this case we didn't err on the side of caution.

For breaking changes, the changelog should contain migration instructions.

We hardly ever do this.

I'll be submitting a bugfix with a sensible default in a moment.

Thank you. Although I do think the current defaults are actually not necessarily "wrong". We don't set accessMode or Size in any of our charts because we want this to be a very explicit choice made by the user and refer them to our storage options docs. For all we know the user wants to use an emptyDir and that won't work well with the other fields.

[morremeyer]

@bjw-s

Good news, turns out the few people that are maintaining these charts are only human after all!.

I've been making this error myself in the past, no worries :) On the contrary, I have to thank everyone who maintains here for their tireless work!

Honestly, I was torn on this, whether to submit a revert or just bump the chart version. If you think bumping the chart version is better or even we leave it like this, I’m fine with all of that. Just wanted to give the option of going „oh yeah right, let's just merge this and move on“ if that's the route you want to take.

[bjw-s]

Apologies for the bit of snark that came through there... Temperatures are pretty high currently, so the snark-fuse got lit a bit quicker than normal 😅 We do really appreciate the feedback, at the very least it reminds us to maybe be even more cautious in the future

[morremeyer]

No worries, I'm melting here myself, Berlin is too hot and I do not have a problem with a bit of snark every now and then.

The important things are that we both now there's no ill intent and do understand each other.

🤞 for lower temperatures tonight, have a great one!