Bug 1891314: Return helm charts based on the installed kubernetes version #7012

akashshinde · 2020-10-28T10:50:36Z

This PR enables support to filter charts based on the kubernetes version.
onlyCompatible flag is provided to /api/helm/charts/index.yaml API which drops the unsupported charts from the response.

Ref: https://issues.redhat.com/browse/HELM-12

pedjak

PR is not implemented according to https://issues.redhat.com/browse/HELM-12 acceptance criteria.

pkg/helm/chartproxy/proxy.go

akashshinde · 2020-11-03T09:42:59Z

/test backend

akashshinde · 2020-11-03T12:14:20Z

/retest

pkg/helm/chartproxy/proxy.go

pkg/helm/handlers/handlers.go

pkg/helm/chartproxy/proxy_test.go

pkg/helm/chartproxy/testdata/mergedAzureRepoIndex.yaml

pkg/helm/handlers/handler_test.go

akashshinde · 2020-11-05T08:53:02Z

/retest

pkg/helm/chartproxy/proxy_test.go

pkg/helm/cluster/kube_version.go

pkg/helm/handlers/handler_test.go

pedjak · 2020-11-05T09:22:31Z

pkg/helm/handlers/handlers.go

@akashshinde ^^

pkg/server/server.go

pedjak · 2020-11-05T10:04:40Z

pkg/helm/chartproxy/testdata/emptyRepoIndex.yaml is empty and not used anywhere..

rohitkrai03 · 2020-11-05T15:29:02Z

@akashshinde @pedjak I would prefer if we make it default that all chart are filtered based on compatibility as this is the direction that we are looking at for Helm catalog by default. It also removes the need to change the UI for this. We can have an option to send all charts unfiltered that UI can use when required but there is no need for fetching unfiltered charts right now so it would be great if we can consider that.

pedjak · 2020-11-05T16:04:43Z

@rohitkrai03 thanks for the feedback.. we can revert the default behaviour...

pedjak

please look at the existing comments, there are still some unaddressed.

pkg/helm/chartproxy/proxy_test.go

pkg/helm/actions/kube_version.go

pkg/helm/cluster/kube_version.go

pkg/helm/handlers/handlers.go

pkg/server/server.go

pkg/helm/chartproxy/testdata/mergedAzureRepoIndex.yaml

docs/helm/endpoints_api.md

pedjak · 2020-11-06T09:58:24Z

/retitle Bug 1891314: Return helm charts based on the installed kubernetes version

openshift-ci-robot · 2020-11-06T09:58:50Z

@akashshinde: This pull request references Bugzilla bug 1891314, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1891314: Return helm charts based on the installed kubernetes version

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.

pedjak · 2020-11-06T11:13:58Z

/cherry-pick release-4.6

openshift-cherrypick-robot · 2020-11-06T11:13:58Z

@pedjak: once the present PR merges, I will cherry-pick it on top of release-4.6 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-4.6

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.

rohitkrai03 · 2020-11-12T10:41:38Z

pkg/helm/chartproxy/testdata/RepoIndexPreRelease.yaml

Usually a helm chart that support pre-release versions specifies kubeVersion range with a -0 at the end. Below is a helm chart metadata from our catalog that support pre-release version -

- apiVersion: v2 appVersion: 10.0.0 created: "2020-11-03T14:58:13.7979+05:30" dependencies: - alias: sch name: ibm-sch repository: '@sch' version: 1.2.15 description: |- IBM Order management Software Enterprise Edition v10 provides cross-channel order orchestration capabilities to enable intelligent brokering of orders across many disparate systems. It also provides a global view of inventory across the supply chain and enables business users to make changes to order process. Documentation For additional details regarding install parameters check http://ibm.biz/oms-helm-readme. License By installing this product you accept the license terms http://ibm.biz/oms-license and http://ibm.biz/oms-apps-license. digest: 47f7bae2c23ceec955c497ddd3f1d2adeea08238363a849c3ffc0ebefb9125ff home: http://ibm.biz/oms-home icon: https://raw.githubusercontent.com/IBM/charts/master/logo/ibm-oms-logo.png keywords: - oms - sterling - yantra - order - fulfillment - om - amd64 - ppc64le - framework - Commercial - RHOCP - Other kubeVersion: '>=1.16.0-0' maintainers: - name: IBM name: ibm-oms-ent-prod type: application urls: - https://redhat-developer.github.com/redhat-helm-charts/charts/ibm-oms-ent-prod-5.1.0.tgz version: 5.1.0

Added 81e25d7
cc: @rohitkrai03

rohitkrai03 · 2020-11-17T07:59:17Z

/assign

rohitkrai03 · 2020-11-17T08:31:14Z

@akashshinde I created a 4.6.0-0.nightly-2020-09-03-191144 cluster to test this PR. It's the same cluster that the bug was reported.

❯ oc version
Client Version: 4.5.5
Server Version: 4.6.0-0.nightly-2020-09-03-191144
Kubernetes Version: v4.6.0-202009031349.p0-dirty

Added below HelmChartRepository CR -

apiVersion: helm.openshift.io/v1beta1
kind: HelmChartRepository
metadata:
  name: ibm-sample-repo
spec:
  name: ibm-sample-repo
  connectionConfig:
    url: https://raw.githubusercontent.com/IBM/charts/master/repo/ibm-helm

The repo has in total 38 charts in it's index.yaml. Out of 38 only 5 specifies kubeVersion with -0 at the end which means only 5 support pre release kubeVersion.

Right now in the console i am getting all 38 Helm charts from the backend. So technically the above reported bug is fixed but I see the same number of charts in a cluster where no filtering is available so the filtering might not be working as expected. On a pre-release kubernetes cluster I would expect only those charts to show up which have -0 specified.

Not sure how the filtering it working at the moment but i think there might be some issue here with the filtering. If you want i can share the cluster with you.

akashshinde · 2020-11-17T09:05:17Z

On a pre-release kubernetes cluster I would expect only those charts to show up which have -0 specified.

If kubernetes pre-release cluster version is v1.19.1-beta.3 then following charts can be listed.

kube >= v1.19.0
kube >= v1.19.1
kube >= v1.19.1-0
kube == v1.19.1-beta3

rohitkrai03 · 2020-11-17T09:25:43Z

After discussions with @akashshinde it seems the current filtering logic should work for all of our use cases. The above shared cluster has something wrong because it shows Kubernetes Version: v4.6.0-202009031349.p0-dirty which is not possible.

rohitkrai03

/lgtm

openshift-merge-robot · 2020-11-17T10:31:44Z

@akashshinde: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/kubevirt-plugin	83cf92ef6db9f0b8d68d4e0560f8c62706bf1eca	link	`/test kubevirt-plugin`

Full PR test history. Your PR dashboard.

Details

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.

pedjak · 2020-11-17T14:04:39Z

/approve

pedjak · 2020-11-17T14:04:51Z

/assign @spadgett

akashshinde · 2020-11-18T11:21:13Z

/retest

akashshinde · 2020-11-19T04:16:47Z

@spadgett Can you take a look at this ? There are few changes in server.go which requires your attention.

akashshinde · 2020-11-30T08:49:27Z

@christianvogt can you please take a look at this ?

spadgett · 2020-11-30T14:19:01Z

pkg/server/server.go

We'll want to make sure this is logged by default. @jhadvig Do you know offhand?

We should fall back to the env var at least on failure.

spadgett · 2020-11-30T14:22:54Z

pkg/server/server.go

I guess it's OK to check only once on startup since the API server is upgraded before the other cluster operators. There might be a small window during upgrades where we show charts for the previous version after the API servers are upgraded.

Note that I believe the kube version is available as an environment variable in the pod, so we could conceivably avoid needing to make this request (although not for off-cluster mode).

@spadgett Getting kube version from env var KUBE_GIT_VERSION in case api server failed to report. Done 739ad74

…led to get it from apiserver

spadgett

/lgtm

openshift-ci-robot · 2020-12-02T14:24:21Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [spadgett]

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

openshift-ci-robot · 2020-12-02T17:13:49Z

@akashshinde: All pull requests linked via external trackers have merged:

openshift/console#7012

Bugzilla bug 1891314 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1891314: Return helm charts based on the installed kubernetes version

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.

openshift-cherrypick-robot · 2020-12-02T17:14:10Z

@pedjak: #7012 failed to apply on top of branch "release-4.6":

Applying: Filter out helm charts based on kubernetes version
Using index info to reconstruct a base tree...
M	pkg/helm/chartproxy/proxy.go
M	pkg/helm/handlers/handlers.go
M	pkg/server/server.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/server/server.go
CONFLICT (content): Merge conflict in pkg/server/server.go
Auto-merging pkg/helm/handlers/handlers.go
CONFLICT (content): Merge conflict in pkg/helm/handlers/handlers.go
Auto-merging pkg/helm/chartproxy/proxy.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Filter out helm charts based on kubernetes version
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Details

In response to this:

/cherry-pick release-4.6

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.

openshift-ci-robot requested review from pedjak and sbose78 October 28, 2020 10:50

openshift-ci-robot added component/backend Related to backend approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 28, 2020

pedjak suggested changes Oct 30, 2020

View reviewed changes

pkg/helm/chartproxy/proxy.go Outdated Show resolved Hide resolved

pkg/helm/chartproxy/proxy.go Outdated Show resolved Hide resolved

pkg/helm/chartproxy/proxy.go Outdated Show resolved Hide resolved

pkg/helm/chartproxy/proxy.go Outdated Show resolved Hide resolved

akashshinde force-pushed the filter-charts branch from 9c6a7cf to 8a605eb Compare November 3, 2020 08:23

pedjak suggested changes Nov 3, 2020

View reviewed changes

openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2020

pedjak suggested changes Nov 5, 2020

View reviewed changes

pedjak suggested changes Nov 6, 2020

View reviewed changes

pkg/helm/chartproxy/proxy_test.go Outdated Show resolved Hide resolved

pkg/helm/chartproxy/proxy_test.go Outdated Show resolved Hide resolved

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2020

akashshinde force-pushed the filter-charts branch from 44afe28 to 500e7a2 Compare November 6, 2020 07:06

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2020

pedjak suggested changes Nov 6, 2020

View reviewed changes

openshift-ci-robot changed the title ~~Return helm charts based on the installed kubernetes version.~~ Bug 1891314: Return helm charts based on the installed kubernetes version Nov 6, 2020

openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Nov 6, 2020

openshift-ci-robot added component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/dashboard Related to dashboard labels Nov 9, 2020

rohitkrai03 suggested changes Nov 12, 2020

View reviewed changes

openshift-ci-robot assigned rohitkrai03 Nov 17, 2020

akashshinde force-pushed the filter-charts branch from 81e25d7 to c0040ab Compare November 17, 2020 09:25

rohitkrai03 approved these changes Nov 17, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2020

openshift-ci-robot assigned spadgett Nov 17, 2020

spadgett reviewed Nov 30, 2020

View reviewed changes

akashshinde added 2 commits December 1, 2020 12:40

Filter out helm charts based on kubernetes version

90f1269

Fallback to kube version from the environment variable in case of fai…

739ad74

…led to get it from apiserver

akashshinde force-pushed the filter-charts branch from c0040ab to 739ad74 Compare December 1, 2020 07:25

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2020

spadgett approved these changes Dec 2, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 2, 2020

openshift-merge-robot merged commit 762a721 into openshift:master Dec 2, 2020

spadgett added this to the v4.7 milestone Dec 9, 2020

pedjak mentioned this pull request Jan 15, 2021

Bug 1916860: Filter out helm charts based on kubernetes version #7851

Merged

Bug 1891314: Return helm charts based on the installed kubernetes version #7012

Bug 1891314: Return helm charts based on the installed kubernetes version #7012

Uh oh!

Conversation

akashshinde commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedjak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akashshinde commented Nov 3, 2020

Uh oh!

akashshinde commented Nov 3, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akashshinde commented Nov 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pedjak commented Nov 5, 2020

Uh oh!

rohitkrai03 commented Nov 5, 2020

Uh oh!

pedjak commented Nov 5, 2020

Uh oh!

pedjak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pedjak commented Nov 6, 2020

Uh oh!

openshift-ci-robot commented Nov 6, 2020

Uh oh!

pedjak commented Nov 6, 2020

Uh oh!

openshift-cherrypick-robot commented Nov 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rohitkrai03 commented Nov 17, 2020

Uh oh!

rohitkrai03 commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akashshinde commented Nov 17, 2020

Uh oh!

rohitkrai03 commented Nov 17, 2020

akashshinde commented Oct 28, 2020 •

edited

Loading

rohitkrai03 commented Nov 17, 2020 •

edited

Loading

openshift-merge-robot commented Nov 17, 2020 •

edited

Loading