Detect major dependency version bumps #571

Merged
justusbunsi merged 6 commits from justusbunsi/helm-chart:409-detect-major-version-bumps into main 2023-11-27 18:36:47 +00:00
Member

As seen in #507 and #569, there is no guarantee for us that minor
dependency updates are actually minor updates for the dependent
application itself. The Chart version might be minor - and therefore
automatically merged when build is green - but the used Docker image
inside the Chart could still be a major version change.

To effectively prevent such automerge when the application major version
changes, there is now a test file that has the currently used major
versions hard-coded. In case of an actual major bump, this file has to
be adjusted.

Looking at redis-cluster, there might be several major Chart versions
with the same major application version.

This PR is related to #409 but does not fully resolve it.

As seen in #507 and #569, there is no guarantee for us that minor dependency updates are actually minor updates for the dependent application itself. The Chart version might be minor - and therefore automatically merged when build is green - but the used Docker image inside the Chart could still be a major version change. To effectively prevent such automerge when the application major version changes, there is now a test file that has the currently used major versions hard-coded. In case of an actual major bump, this file has to be adjusted. Looking at `redis-cluster`, there might be several major Chart versions with the same major application version. This PR is related to #409 but does not fully resolve it.
justusbunsi added 1 commit 2023-11-18 13:27:41 +00:00
All checks were successful
check-and-test / check-and-test (pull_request) Successful in 30s
e4da2e1b6d
Detect major dependency version bumps
As seen in #507 and #569, there is no guarantee for us that minor
dependency updates are actually minor updates for the dependent
application itself. The Chart version might be minor - and therefore
automatically merged when build is green - but the used Docker image
inside the Chart could still be a major version change.

To effectively prevent such automerge when the application major version
changes, there is now a test file that has the currently used major
versions hard-coded. In case of an actual major bump, this file has to
be adjusted.

Looking at `redis-cluster`, there might be several major Chart versions
with the same major application version.

This PR is related to #409 but does not fully resolve it.

Signed-off-by: justusbunsi <sk.bunsenbrenner@gmail.com>
justusbunsi reviewed 2023-11-18 13:31:04 +00:00
@ -10,3 +10,3 @@
.PHONY: unittests
unittests:
helm unittest --strict -f 'unittests/**/*.yaml' ./
helm unittest --strict -f 'unittests/**/*.yaml' -f 'unittests/dependency-major-image-check.yaml' ./
Author
Member

I added the major image check at the end of the suite so that it would be the last test file to run. The current v3.0.6 of helm-unittest suffers from an excessive log output1 which obscures the test results and failures in our builds.

I added the major image check at the end of the suite so that it would be the last test file to run. The current `v3.0.6` of helm-unittest suffers from an excessive log output[^1] which obscures the test results and failures in our builds. [^1]: https://github.com/helm-unittest/helm-unittest/issues/237
justusbunsi added 1 commit 2023-11-18 13:32:06 +00:00
Some checks failed
check-and-test / check-and-test (pull_request) Failing after 7s
bb9cb26686
Intentionally break the build to show how it looks
Signed-off-by: justusbunsi <sk.bunsenbrenner@gmail.com>
justusbunsi added 1 commit 2023-11-18 13:33:57 +00:00
Some checks failed
check-and-test / check-and-test (pull_request) Failing after 29s
7f57151c54
Intentionally break the build - part 2
Signed-off-by: justusbunsi <sk.bunsenbrenner@gmail.com>
Author
Member

Major-change failures look like this:

image

Major-change failures look like this: ![image](/attachments/cb122f51-aa55-41b6-a2c8-5c5ee4494853)
justusbunsi added 2 commits 2023-11-18 13:36:31 +00:00
All checks were successful
check-and-test / check-and-test (pull_request) Successful in 31s
a1fe73bbb7
Revert "Intentionally break the build to show how it looks"
This reverts commit bb9cb26686.
justusbunsi added the
skip-changelog
label 2023-11-18 13:47:26 +00:00
justusbunsi changed title from WIP: Detect major dependency version bumps to Detect major dependency version bumps 2023-11-18 15:20:06 +00:00
justusbunsi requested review from pat-s 2023-11-18 15:20:13 +00:00
pat-s approved these changes 2023-11-18 17:05:09 +00:00
pat-s left a comment
Member

Awesome, thanks!

Looking at redis-cluster, there might be several major Chart versions
with the same major application version.

Yes, if there are major changes in the chart, there will be a major version bump as well. So it could be for both reasons, major application version update or chart version.

Good to have the security now to prevent issues like #507!

I also thought in more detail how we could go with the bitnami chart updates, i.e. we could ensure staying on the latest chart version but encouraging users to fix their image tag to a version compatible with their actual PG version.

This has two benefits:

  • new users always start with the latest major application version
  • Users can decide on their own when they do the upgrade
  • We don't fall behind chart changes

But more on this soon...

Awesome, thanks! > Looking at redis-cluster, there might be several major Chart versions with the same major application version. Yes, if there are major changes in the chart, there will be a major version bump as well. So it could be for both reasons, major application version update or chart version. Good to have the security now to prevent issues like #507! I also thought in more detail how we could go with the bitnami chart updates, i.e. we could ensure staying on the latest chart version but encouraging users to fix their image tag to a version compatible with their actual PG version. This has two benefits: - new users always start with the latest major application version - Users can decide on their own when they do the upgrade - We don't fall behind chart changes But more on this soon...
Member

With this PR merged, are we ready to enable branch automerging for minor & friends?

And do we really want to "skip changelog" for such features? Sure they are chart internal but I stil think they are worth being mentioned in a changelog.

With this PR merged, are we ready to enable branch automerging for minor & friends? And do we really want to "skip changelog" for such features? Sure they are chart internal but I stil think they are worth being mentioned in a changelog.
Author
Member

Good point on the labels. Let's remove them on both my PRs from today.

Regarding automerge: From what I see there are still some things to ensure for #409 really be resolved. E.g that our mapping for ports has an effect. In general, all subcharts settings we have in our values should be verified being templated correctly. We had regressions in the past I would like to prevent that for the future. Otherwise we always go through the subchart changes ourself to see what changed.
If users add their own subchart values, that's out of our scope. Those we set are our responsibility to check.

Good point on the labels. Let's remove them on both my PRs from today. Regarding automerge: From what I see there are still some things to ensure for #409 really be resolved. E.g that our mapping for ports has an effect. In general, all subcharts settings _we_ have in our values should be verified being templated correctly. We had regressions in the past I would like to prevent that for the future. Otherwise we always go through the subchart changes ourself to see what changed. If users add their own subchart values, that's out of our scope. Those _we_ set are our responsibility to check.
justusbunsi removed the
skip-changelog
label 2023-11-18 17:18:34 +00:00
justusbunsi added 1 commit 2023-11-27 18:35:57 +00:00
All checks were successful
check-and-test / check-and-test (pull_request) Successful in 29s
42ea751c46
Merge branch 'main' into 409-detect-major-version-bumps
justusbunsi merged commit 8bcd2dc63b into main 2023-11-27 18:36:47 +00:00
justusbunsi deleted branch 409-detect-major-version-bumps 2023-11-27 18:36:48 +00:00
Sign in to join this conversation.
No description provided.