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

Disable helm CRD installation for disable-helm-controller #8702

Conversation

harsimranmaan
Copy link
Contributor

@harsimranmaan harsimranmaan commented Oct 19, 2023

The NewContext package requires config as input which would require all third-party callers to update when the new go module is published.

This change only affects the behaviour of installation of helm CRDs. Existing helm crds installed in a cluster would not be removed when disable-helm-controller flag is set on the server.

Addresses #8701

Proposed Changes

Disable helm CRD installation for disable-helm-controller

Types of Changes

Breaking Change (for sdk) not for k3s service

Verification

Testing

Linked Issues

User-Facing Change


Further Comments

Closes #8701

The NewContext package requires config as input which would
require all third-party callers to update when the new go module
is published.

This change only affects the behaviour of installation of helm
CRDs. Existing helm crds installed in a cluster would not be removed
when disable-helm-controller flag is set on the server.

Addresses k3s-io#8701

Signed-off-by: Harsimran Singh Maan <maan.harry@gmail.com>
@harsimranmaan harsimranmaan requested a review from a team as a code owner October 19, 2023 21:35
Copy link
Contributor

@brandond brandond left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

We try to avoid passing in both a struct, and a field on that struct. Just pass in the Config struct, and use the forServer flag to determine which config to use.

pkg/cli/etcdsnapshot/etcd_snapshot.go Outdated Show resolved Hide resolved
pkg/server/context.go Outdated Show resolved Hide resolved
pkg/server/context.go Outdated Show resolved Hide resolved
pkg/server/context.go Outdated Show resolved Hide resolved
pkg/server/context.go Outdated Show resolved Hide resolved
pkg/server/server.go Outdated Show resolved Hide resolved
Signed-off-by: Harsimran Singh Maan <maan.harry@gmail.com>
Signed-off-by: Harsimran Singh Maan <maan.harry@gmail.com>
@brandond
Copy link
Contributor

LGTM! We can squash when merging.

@harsimranmaan
Copy link
Contributor Author

Hi @brandond, lemme know if this require any further work. Feel free to merge it

@brandond
Copy link
Contributor

You're all set.We're in code freeze pending release of last week's patches. We will merge things again once the releases are out.

@brandond
Copy link
Contributor

arm and s390x are flaking; merging

@brandond brandond merged commit abc2efd into k3s-io:master Nov 15, 2023
15 of 18 checks passed
brandond pushed a commit to brandond/k3s that referenced this pull request Nov 16, 2023
* Disable helm CRD installation for disable-helm-controller
    The NewContext package requires config as input which would
    require all third-party callers to update when the new go module
    is published.

    This change only affects the behaviour of installation of helm
    CRDs. Existing helm crds installed in a cluster would not be removed
    when disable-helm-controller flag is set on the server.

    Addresses k3s-io#8701
* address review comments
* remove redundant check

Signed-off-by: Harsimran Singh Maan <maan.harry@gmail.com>
(cherry picked from commit abc2efd)
Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond pushed a commit to brandond/k3s that referenced this pull request Nov 16, 2023
* Disable helm CRD installation for disable-helm-controller
    The NewContext package requires config as input which would
    require all third-party callers to update when the new go module
    is published.

    This change only affects the behaviour of installation of helm
    CRDs. Existing helm crds installed in a cluster would not be removed
    when disable-helm-controller flag is set on the server.

    Addresses k3s-io#8701
* address review comments
* remove redundant check

Signed-off-by: Harsimran Singh Maan <maan.harry@gmail.com>
(cherry picked from commit abc2efd)
Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond pushed a commit to brandond/k3s that referenced this pull request Nov 16, 2023
* Disable helm CRD installation for disable-helm-controller
    The NewContext package requires config as input which would
    require all third-party callers to update when the new go module
    is published.

    This change only affects the behaviour of installation of helm
    CRDs. Existing helm crds installed in a cluster would not be removed
    when disable-helm-controller flag is set on the server.

    Addresses k3s-io#8701
* address review comments
* remove redundant check

Signed-off-by: Harsimran Singh Maan <maan.harry@gmail.com>
(cherry picked from commit abc2efd)
Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond pushed a commit that referenced this pull request Nov 16, 2023
* Disable helm CRD installation for disable-helm-controller
    The NewContext package requires config as input which would
    require all third-party callers to update when the new go module
    is published.

    This change only affects the behaviour of installation of helm
    CRDs. Existing helm crds installed in a cluster would not be removed
    when disable-helm-controller flag is set on the server.

    Addresses #8701
* address review comments
* remove redundant check

Signed-off-by: Harsimran Singh Maan <maan.harry@gmail.com>
(cherry picked from commit abc2efd)
Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond pushed a commit that referenced this pull request Nov 16, 2023
* Disable helm CRD installation for disable-helm-controller
    The NewContext package requires config as input which would
    require all third-party callers to update when the new go module
    is published.

    This change only affects the behaviour of installation of helm
    CRDs. Existing helm crds installed in a cluster would not be removed
    when disable-helm-controller flag is set on the server.

    Addresses #8701
* address review comments
* remove redundant check

Signed-off-by: Harsimran Singh Maan <maan.harry@gmail.com>
(cherry picked from commit abc2efd)
Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
brandond pushed a commit that referenced this pull request Nov 16, 2023
* Disable helm CRD installation for disable-helm-controller
    The NewContext package requires config as input which would
    require all third-party callers to update when the new go module
    is published.

    This change only affects the behaviour of installation of helm
    CRDs. Existing helm crds installed in a cluster would not be removed
    when disable-helm-controller flag is set on the server.

    Addresses #8701
* address review comments
* remove redundant check

Signed-off-by: Harsimran Singh Maan <maan.harry@gmail.com>
(cherry picked from commit abc2efd)
Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
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.

Helm CRDS are installed despite turning off helm-controller
3 participants