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

helm: add a new namespaced CRD for helm #1084

Merged

Conversation

zonggen
Copy link

@zonggen zonggen commented Dec 13, 2021

As part of the process to support namespace-scoped Helm repositories for
non-admin users, this PR adds a new namespace-scoped CRD definition
named projecthelmchartrepositories.helm.openshift.io.

Enhancement (under review): openshift/enhancements#949
Closes: https://issues.redhat.com/browse/HELM-258
Signed-off-by: Allen Bai abai@redhat.com

@zonggen
Copy link
Author

zonggen commented Dec 13, 2021

/hold
Until the enhancement is approved and merged.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 13, 2021
@zonggen
Copy link
Author

zonggen commented Dec 14, 2021

Not sure if this is related, but make verify-scripts was passing locally for me. Ran with go mod tidy, go mod vendor, followed by make verify-scripts.

$ go version 
go version go1.17.5 linux/amd64

Copy link

@dperaza4dustbit dperaza4dustbit left a comment

Choose a reason for hiding this comment

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

LGTM

@zonggen
Copy link
Author

zonggen commented Dec 14, 2021

@pedjak Could you also take a look at this change? Tagging you because you were the author of last Helm CRD change.

@zonggen
Copy link
Author

zonggen commented Jan 6, 2022

Since the enhancement is merged openshift/enhancements#949,

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 6, 2022
//
// Compatibility level 2: Stable within a major release for a minimum of 9 months or 3 minor releases (whichever is longer).
// +openshift:compatibility-gen:level=2
type ProjectHelmChartRepository struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that ProjectHelmChartRepository is just a namespaced version of HelmChartRepository? If yes, we should keep them in sync, and to avoid large copy/paste blocks, we could do the following:

// +genclient
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
// +kubebuilder:plural=projecthelmchartrepositories

// ProjectHelmChartRepository holds namespace-wide configuration for proxied Helm chart repository
//
// Compatibility level 2: Stable within a major release for a minimum of 9 months or 3 minor releases (whichever is longer).
// +openshift:compatibility-gen:level=2
type ProjectHelmChartRepository struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata,omitempty"`

	// spec holds user settable values for configuration
	// +kubebuilder:validation:Required
	// +required
	Spec HelmChartRepositorySpec `json:"spec"`

	// Observed status of the repository within the namespace..
	// +optional
	Status HelmChartRepositoryStatus `json:"status"`
}

// Compatibility level 2: Stable within a major release for a minimum of 9 months or 3 minor releases (whichever is longer).
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
// +openshift:compatibility-gen:level=2
type ProjectHelmChartRepositoryList struct {
	metav1.TypeMeta `json:",inline"`
	metav1.ListMeta `json:"metadata"`

	Items []ProjectHelmChartRepository `json:"items"`
}

With the above, we can even keep these two structs in `types_helm.go` file, along with the other types.

@pedjak
Copy link
Contributor

pedjak commented Jan 10, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2022
@openshift-ci openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. labels Jan 10, 2022
As part of the process to support namespace-scoped Helm repositories for
non-admin users, this PR adds a new namespace-scoped CRD definition
named `projecthelmchartrepositories.helm.openshift.io`.

Closes: https://issues.redhat.com/browse/HELM-258
Signed-off-by: Allen Bai <abai@redhat.com>
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2022
@zonggen zonggen force-pushed the helm-258-namespace-crd branch 2 times, most recently from 80b6b77 to 5c9e162 Compare January 10, 2022 20:44
Remove duplicated status and spec struct for namespace scoped helm chart
repos, instead using the previous structs.

Signed-off-by: Allen Bai <abai@redhat.com>
@pedjak
Copy link
Contributor

pedjak commented Jan 10, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2022
@zonggen
Copy link
Author

zonggen commented Jan 11, 2022

/assign @knobunc

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 12, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pedjak, spadgett, zonggen

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2022
@debsmita1
Copy link
Contributor

/label docs-approved
/label px-approved

@openshift-ci openshift-ci bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Jan 12, 2022
@knobunc
Copy link
Contributor

knobunc commented Jan 12, 2022

I think this is more for @bparees than me?

/assign @bparees

@knobunc knobunc removed their assignment Jan 12, 2022
@bparees
Copy link
Contributor

bparees commented Jan 12, 2022

this PR is attempting to land post feature freeze, which means it needs to be going through the no-FF process, which in this case means it is awaiting the qe-approved label based on QE testing this feature prior to allowing it to merge.

(of course this PR is only one piece of the feature, so links to the related PRs would help QE w/ that process).

and all that assumes that the team delivering this work is a no-FF process team.

@tisutisu
Copy link

tisutisu commented Jan 17, 2022

Tested this changes with OCP 4.10 dev cluster, Below steps are working fine.

  1. Create the CRD
$ oc apply -f helm/v1beta1/0000_10-project-helm-chart-repository.crd.yaml
customresourcedefinition.apiextensions.k8s.io/projecthelmchartrepositories.helm.openshift.io created

$ oc get crd | grep helm
helmchartrepositories.helm.openshift.io                           2022-01-13T07:27:25Z
projecthelmchartrepositories.helm.openshift.io                    2022-01-13T14:38:33Z 
  1. Create a CR instance, like in https://github.com/openshift/console/pull/10467/files?short_path=9792ce1#diff-9792ce17aec3657e6f182e08e62ce981cfbcfc33f70cfde0b6f2c40beec672d7

@debsmita1
Copy link
Contributor

Adding the qe approval on @tisutisu 's behalf
/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jan 17, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 17, 2022

@zonggen: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 5bc1779 into openshift:master Jan 17, 2022
@zonggen
Copy link
Author

zonggen commented Feb 1, 2022

NOTE: used make generate-with-container to update the deepcopy files.

zonggen added a commit to zonggen/console-operator that referenced this pull request Feb 1, 2022
Refer: openshift/api#1084
Signed-off-by: Allen Bai <carpe.diem.allen@gmail.com>
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/console-operator that referenced this pull request Feb 14, 2022
Refer: openshift/api#1084
Signed-off-by: Allen Bai <carpe.diem.allen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants