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

fix(Helm.controller.watch): Process watch for List kind #4682

Merged
merged 1 commit into from
Mar 18, 2021

Conversation

hickeyma
Copy link
Contributor

@hickeyma hickeyma commented Mar 18, 2021

Description of the change:

The Kubernetes kind: List is not actually a resource and therefore cannot have a watch set on it. This PR iterates on the List items instead and sets the watch on those Kubernetes kinds.

Motivation for the change:

Helm operator controller errors out when a Helm chart has a kind: List declared.

Closes #4636

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:09 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:09 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:09 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:09 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:09 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:09 Inactive
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

Just some nits and a suggested fix. Otherwise LGTM. Thanks @hickeyma!

internal/helm/controller/controller.go Outdated Show resolved Hide resolved
internal/helm/controller/controller.go Outdated Show resolved Hide resolved
internal/helm/controller/controller.go Outdated Show resolved Hide resolved
@estroz
Copy link
Member

estroz commented Mar 18, 2021

Additionally can you add a changelog entry for this bugfix?

/kind bug

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 18, 2021
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:34 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:34 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:34 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:34 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:34 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:34 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:46 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:46 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:46 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:46 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:46 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 20:46 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 21:21 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 21:21 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 21:21 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 21:21 Inactive
@hickeyma
Copy link
Contributor Author

Thanks fo the review @estroz. I have updated with your feedback and it is ready to review again.

BTW, does this need a changelog?

@estroz
Copy link
Member

estroz commented Mar 18, 2021

BTW, does this need a changelog?

Yep!

@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 21:53 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 21:53 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 21:53 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 21:53 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 21:53 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 21:53 Inactive
Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
@hickeyma
Copy link
Contributor Author

@estroz Changelog added.

@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 21:57 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 21:57 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 21:57 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 21:57 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 21:57 Inactive
@hickeyma hickeyma temporarily deployed to deploy March 18, 2021 21:57 Inactive
@hickeyma hickeyma requested a review from estroz March 18, 2021 22:24
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

Thanks again @hickeyma

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 18, 2021
@estroz estroz merged commit 7f6a53c into operator-framework:master Mar 18, 2021
@hickeyma
Copy link
Contributor Author

Thanks for reviews and merge @estroz

@hickeyma hickeyma deleted the fix/handle-list-in-watch branch March 18, 2021 22:31
varshaprasad96 added a commit to varshaprasad96/helm-operator-plugins that referenced this pull request Dec 2, 2021
This commit modifies `dependentResourceWatcher` to
accept `List` kinds while watching dependent resources.
It also adds test to verify if `setWatchOnResource`
works for lists.

Reference: operator-framework/operator-sdk#4682

Signed-off-by: varshaprasad96 <varshaprasad96@gmail.com>
varshaprasad96 added a commit to operator-framework/helm-operator-plugins that referenced this pull request Dec 6, 2021
This commit modifies `dependentResourceWatcher` to
accept `List` kinds while watching dependent resources.
It also adds test to verify if `setWatchOnResource`
works for lists.

Reference: operator-framework/operator-sdk#4682

Signed-off-by: varshaprasad96 <varshaprasad96@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helm Operator doesn't work with Kube Prometheus Stack with additionalPrometheusRulesMap yaml list
3 participants