-
Notifications
You must be signed in to change notification settings - Fork 20
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
replace all ${VAR} references with env var values in watches yaml #45
replace all ${VAR} references with env var values in watches yaml #45
Conversation
Signed-off-by: John Mazzitelli <mazz@redhat.com>
@@ -892,3 +892,49 @@ func TestGetPossibleRolePaths(t *testing.T) { | |||
}) | |||
} | |||
} | |||
|
|||
func TestReplaceEnvVars(t *testing.T) { //nolint:gocyclo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on the reasoning for the nolint here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy-paste :) I copied the TestLoad function when I started writing my test function, and it has that. No reason other than that. I can delete that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is done in latest commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me. Couple comments.
Addressed all the suggestions. As for release notes, I would say we need to mention that odd edge case where
|
We currently have autogenerated release notes in this repository, but once the changes are pulled into the Operator-SDK we can provide a more detailed release note entry. Thanks for the contribution @jmazzitelli ! |
Description of the change:
For any ${VAR} references in the watches yaml, this replaces those env vars with their values before the watches yaml is processed. If there is no ${VAR} defined, the "${VAR}" remains as-is in the watches yaml.
Motivation for the change:
There are times when a user wants to customize what an operator will watch. Right now, watches.yaml is fixed and cannot be customized or configured. This feature at least allows the operator to be somewhat configurable by passing in env vars in the operator's subscription or directly in its Deployment.
For one such example, I may have an operator that can be deployed on OpenShift and non-OpenShift environments. I therefore could have two different sets of playbooks in my operator container - one for OpenShift and one for non-OpenShift. I could define a set of ${WATCH_PLAYBOOK_XYZ} env vars and point to cluster-specific playbooks in the watches.yaml:
When deployed on OpenShift, my OLM CSV could define that env var to "playbook-bar-openshift.yml" and on non-OpenShift, I have its OLM CSV define the env var to "playbook-bar-kubernetes.yml".
Another example. I want my operator to watch Bar objects, but I do NOT want the operator to run my playbook reconciliation on every Bar in the cluster - I only want to run a reconciliation when a Bar with a specific selector. But! (and here's the important part), this selector can be different depending on what the admin wants (the admin being the person who installed the operator). With this PR, this can be accomplished like this:
watches.yaml:
Operator Subscription: