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

pkg/helm: new watch file option to support watching dependent resources #916

Merged

Conversation

joelanford
Copy link
Member

Description of the change:

  • Adds a new option to the watches.yaml file called watchDependentResources, which enables users to configure whether resources defined by Helm releases are watched for reconciliation. This option is enabled by default. This is the same mechanism as the ansible-operator to maintain consistency.
  • Decouples the loading of the watch file from the creation of manager factories by refactoring the loading of the watch file into a separate watches package. This makes it easier to add new watch configurations that are outside the scope of the release manager factory.
  • Removes helpers that allowed watches to be configured from environment variables. Now that multiple watches are supported and new watch-specific configurations are being added, loading from the environment does not provide the necessary flexibility.

Motivation for the change:
See #885 (comment).

This commit adds a new option to the watches.yaml file called
`watchDependentResources`, which enables users to configure whether
resources defined by Helm releases are watched for reconciliation.

This commit also decouples the loading of the watch file from the
creation of manager factories by refactoring the loading of the watch
file into a separate `watches` package. This makes it easier to add new
watch configurations that are outside the scope of the release manager
factory.

Lastly, this commit removes helpers that allowed watches to be
configured from environment variables. Now that multiple watches are
supported and new watch-specific configurations are being added, loading
from the environment does not provide the necessary flexibility.
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 14, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2019
@joelanford
Copy link
Member Author

joelanford commented Jan 17, 2019

Chatting with @robszumski, I realized there's still value in the helm-app-operator-kit UX that made it really easy to build an operator from a Helm chart using just environment variables and a Dockerfile.

The operator-sdk CLI doesn't currently expose a way to make use of these environment variables (which are removed in this PR), so I don't think we'll regress the UX by merging this PR (we unknowingly regressed it already).

Having said that, we need to think about how to simplify building operators from existing Helm charts. I'd propose that we go ahead with this PR as is and draft a proposal to implement Rob's ideas for making it easier to build Helm (and Ansible?) operator images with new CLI commands/flags.

EDIT: See #949 for the proposed UX improvement.

Copy link
Member

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

LGTM

@joelanford joelanford merged commit 5f3c3a5 into operator-framework:master Feb 4, 2019
@joelanford joelanford deleted the helm-dependent-watch-option branch March 1, 2019 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants