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

workflows: update helm release trigger, add helm chart testing, update image.tag on zigbee2mqtt release #17

Closed
wants to merge 0 commits into from

Conversation

tamcore
Copy link
Contributor

@tamcore tamcore commented Oct 31, 2024

To make this possible, we're overriding the containers command and livenessProbe, since it's not possible to get the instance into a health state without having a zigbee adapter available. So we're basically just testing if the chart renders correctly and is installable on the most recent 3 Kubernetes versions.

Also fixed the OnZigbee2MQTTRelease workflow, to also update the .image.tag value (would close #12 then)

@Koenkk Koenkk requested a review from jlpedrosa October 31, 2024 19:46
Copy link
Collaborator

@jlpedrosa jlpedrosa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want/need to make the healthcheck configurable, let's make it in another PR, so we can reason through things more orderly please.

@tamcore
Copy link
Contributor Author

tamcore commented Nov 1, 2024

Split out command and livenessProbe into #18, which is now a pre-requisite of this PR.

@tamcore tamcore requested a review from jlpedrosa November 1, 2024 18:33
Koenkk pushed a commit that referenced this pull request Nov 2, 2024
k8s_version:
- v1.31.0
- v1.30.0
- v1.29.0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to tests those versions, pronably we should also limit the versions supported on Chart.yaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never seen it handled that way. I think testing against the latest 3 kubernetes versions is just fine. Paired with requiring any version that is not considered EOL.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tamcore I think you didn't get my point. I'm trying to say that we should update the chart.yaml to 1.29.0 min version as part of this PR. With your PR, we are kinda supporting it better.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here:

kubeVersion: ">=1.26.0-0"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did understand you. But the value you linked only makes sense, when the chart uses api versions that are not available on older versions. I.e. Ingress bellow 1.19. There's no reason to artificially limit it to exclusively what's being tested. The chart will install just fine in ubernetes 1.21, even though for whatever reason 1.26 is defined as the minimum version.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think makes sense since this is what we are supporting. The same GVK adds some fields in some version (sometimes by gates), I think it make sense that we test what we suport and we put on the manifests what we support.

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.

3 participants