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

feat(helm): add option to disable helm upgrade force flag #4866

Conversation

emosbaugh
Copy link
Contributor

@emosbaugh emosbaugh commented Aug 16, 2024

Description

Adds the option disableForceUpgrade to the helm extensions chart spec.

When set to true, the "helm upgrade --force" flag is not longer used when upgrading the the chart.

Fixes #4864

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

  • Manual test
  • Auto test added
ethan@ethanm-ec-1:~/build/k0s$ sudo -E helm history -n registry docker-registry
WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /var/lib/k0s/pki/admin.conf
REVISION	UPDATED                 	STATUS  	CHART                	APP VERSION	DESCRIPTION
1       	Fri Aug 16 18:01:44 2024	deployed	docker-registry-2.2.3	2.8.1      	Install complete
ethan@ethanm-ec-1:~/build/k0s$ sudo ./k0s kubectl -n kube-system apply -f k0s.yaml
clusterconfig.k0s.k0sproject.io/k0s configured
ethan@ethanm-ec-1:~/build/k0s$ sudo -E helm history -n registry docker-registry
WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /var/lib/k0s/pki/admin.conf
REVISION	UPDATED                 	STATUS    	CHART                	APP VERSION	DESCRIPTION
1       	Fri Aug 16 18:01:44 2024	superseded	docker-registry-2.2.3	2.8.1      	Install complete
2       	Fri Aug 16 18:04:23 2024	deployed  	docker-registry-2.2.3	2.8.1      	Upgrade complete
  spec:
    extensions:
      helm:
        charts:
        - chartname: twuni/docker-registry
          name: docker-registry
          namespace: registry
          timeout: 0s
          # disableForceUpgrade: true
          values: |
            image:
              tag: 2.8.3
            persistence:
              accessMode: ReadWriteOnce
              enabled: true
              size: 10Gi
              storageClass: local-path
            replicaCount: 1
            storage: filesystem
            # extraEnvVars:
            # - name: TESTING
            #   value: "testing"
          version: 2.2.3
        concurrencyLevel: 5
        repositories:
        - name: twuni
          url: https://helm.twun.io

Checklist:

  • My code follows the style guidelines of this project
  • My commit messages are signed-off
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

@emosbaugh emosbaugh marked this pull request as ready for review August 16, 2024 18:13
@emosbaugh emosbaugh requested a review from a team as a code owner August 16, 2024 18:13
@jnummelin
Copy link
Member

In general I'm in favor for adding option to use/not use force.

The only thing I'm thinking is in which way the option should be defined. I wonder if we could define the flag as useForce (or just plain force) while still defaulting it to true for backwards compatibility? Maybe if we'd define it as a pointer and then check if it was defined or not. 🤔

@emosbaugh
Copy link
Contributor Author

In general I'm in favor for adding option to use/not use force.

The only thing I'm thinking is in which way the option should be defined. I wonder if we could define the flag as useForce (or just plain force) while still defaulting it to true for backwards compatibility? Maybe if we'd define it as a pointer and then check if it was defined or not. 🤔

@jnummelin thank you for the feedback! i've changed the field to a bool pointer forceUpgrade

@emosbaugh emosbaugh force-pushed the issue-4864-helm-extensions-disable-upgrade-force branch from bb38b59 to 2a9ebb8 Compare August 22, 2024 13:17
@emosbaugh
Copy link
Contributor Author

I've forgot to signoff on my commit. Re-pushed.

@emosbaugh
Copy link
Contributor Author

@jnummelin ping here. anything I can do to get this over the finish line?

Copy link
Contributor

This pull request has merge conflicts that need to be resolved.

@jnummelin
Copy link
Member

@emosbaugh unfortunately there's now some conflicts. Give me a ping once you've been able to resolve and I'll review

@emosbaugh emosbaugh force-pushed the issue-4864-helm-extensions-disable-upgrade-force branch from 2a9ebb8 to fe0cacf Compare September 3, 2024 20:02
@emosbaugh
Copy link
Contributor Author

@jnummelin ive rebased

@jnummelin jnummelin added enhancement New feature or request area/helm labels Sep 4, 2024
Version string `json:"version,omitempty"`
Namespace string `json:"namespace,omitempty"`
Timeout string `json:"timeout,omitempty"`
ForceUpgrade *bool `json:"forceUpgrade,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Should we have the defaulting with kubebuilder here too, something like:

	// +kubebuilder:default=true
	// +optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @jnummelin. Updated

Copy link
Contributor

github-actions bot commented Sep 5, 2024

This pull request has merge conflicts that need to be resolved.

Signed-off-by: Ethan Mosbaugh <ethan@replicated.com>
Signed-off-by: Ethan Mosbaugh <ethan@replicated.com>
@emosbaugh emosbaugh force-pushed the issue-4864-helm-extensions-disable-upgrade-force branch from 306f89a to fab233f Compare September 5, 2024 18:50
@emosbaugh emosbaugh force-pushed the issue-4864-helm-extensions-disable-upgrade-force branch from fab233f to 4a1f18a Compare September 6, 2024 12:37
…type

Signed-off-by: Ethan Mosbaugh <ethan@replicated.com>
Signed-off-by: Ethan Mosbaugh <ethan@replicated.com>
Signed-off-by: Ethan Mosbaugh <ethan@replicated.com>
@emosbaugh emosbaugh force-pushed the issue-4864-helm-extensions-disable-upgrade-force branch from 4a1f18a to ffce675 Compare September 6, 2024 12:39
@jnummelin jnummelin merged commit 0eadb0b into k0sproject:main Sep 6, 2024
89 checks passed
@emosbaugh
Copy link
Contributor Author

@jnummelin i think we discussed backporting this to previous versions. is that something that will happen with the next release or something i can help with?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to disable the use of --force flag when running helm upgrade extensions
4 participants