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

Update to 27.0.0 #407

Merged
merged 1 commit into from
Jun 25, 2023
Merged

Update to 27.0.0 #407

merged 1 commit into from
Jun 25, 2023

Conversation

provokateurin
Copy link
Member

Pull Request

Description of the change

Latest NC version

Additional information

Checklist

@provokateurin
Copy link
Member Author

CI seems to fail because of our new Github runners.

Signed-off-by: jld3103 <[email protected]>
@SchoolGuy
Copy link
Contributor

I have heard complaints from colleagues that also run NC instances that 27.0.0 seems to break some apps. Could we instead upgrade to 26.0.3 first and in a month or so merge this PR maybe please?

@provokateurin
Copy link
Member Author

You don't need to deploy the latest helm chart if you don't want 27. You can also just override the image version instead

@provokateurin provokateurin merged commit 7452d8e into main Jun 25, 2023
@delete-merged-branch delete-merged-branch bot deleted the update/27.0.0 branch June 25, 2023 10:54
@jessebot
Copy link
Collaborator

I have heard complaints from colleagues that also run NC instances that 27.0.0 seems to break some apps.

I just wanted to pop in, very late (sorry!), and say yes, it breaks a few apps, but those apps are recovering pretty quickly, and setting your update channel to beta for the apps to get the latest version before setting it back down to stable seems to do the trick. I had the most issues with the Maps app, but that step seemed to have solved it along with deleting a file, but it does seem like we're on a breakthrough with that app too according to nextcloud/maps#1069 Everything in Maps is working as normal for me now.

All other apps have been running ok for me, except Contacts, which produces a weird error when importing contacts and I think is because of nextcloud/server#38772 which should be solved soonish as there's an RC PR here, nextcloud/server#39282, so I assume we'll get a new docker tag soonish.

@provokateurin, my only other note would be in the future, when we change a major appVersion, we should bump the middle number (minor version) of the chart version, instead of the final number (patch version), so 3.5.13 -> 3.6.0, instead of 3.5.13 -> 3.5.14. This just alerts a user that there may be significant changes. An argument could be made to suggest we actually bump the first number (major version) since this could produce a breaking change for the use (e.g. apps not being compatible with the latest version yet). This is to be more compliant with semver. Otherwise though, this has been a welcome change :)

@provokateurin
Copy link
Member Author

@jessebot Your point makes sense, although I would think that semver would always apply to the the chart and not the deployed software. You can use the latest chart and override the image with an old version and it would still work. The update only changes the default version and doesn't introduce breaking changes itself.

To get rid of this problem altogether we could make specifying the image version mandatory. Then we also don't need any updates for new versions. The docker image itself should be already tested by the people who release it. This way we really only care about the chart itself and nothing else. I'm not really a fan of doing this, but it is a possibility.

@jessebot
Copy link
Collaborator

You can use the latest chart and override the image with an old version and it would still work.

That's totally true, but the issue here is that some people will use tools (e.g. renovatebot) to auto-update their helm chart version for patches (this is mostly for security compliance so you can always make sure you get security patches). If we include a major app update in the chart, it would auto-upgrade people's actual nextcloud instances, which may be undesirable. They can always roll forward, but downgrading nextcloud is unsupported and treacherous, so upgrading a minor or major version of the chart on our end, prevents any potential headaches of users that use this in production.

As a quick check for how other established charts do this, I checked out ingress-nginx and it looks like when bumping a minor app version, they bump a minor chart version as well: kubernetes/ingress-nginx@3476232

By contrast, patch versions of the app only get patch version updates of the chart: kubernetes/ingress-nginx@652a800

To get rid of this problem altogether we could make specifying the image version mandatory. Then we also don't need any updates for new versions. The docker image itself should be already tested by the people who release it. This way we really only care about the chart itself and nothing else. I'm not really a fan of doing this, but it is a possibility.

Nah, also not a fan as that's really non-standard. I think we can just start a contributing guide that explicitly mentions that if you bump the appVersion by a minor or major version, please also bump the helm chart version. We should probably work on a CONTRIBUTING.md anyway.

@provokateurin
Copy link
Member Author

Sounds good, I was just playing devil's advocate :D

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