ansible: pull watches into own package #1523
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
size/XL
djzager
force-pushed
the
runner-refactor
branch
3 times, most recently
from
9745fe2
to
3c9491d
Compare
djzager
force-pushed
the
runner-refactor
branch
2 times, most recently
from
29c3afe
to
3ccaa62
Compare
djzager
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
pkg/ansible/watches/watches.go
Outdated
| return err | ||
| } | ||
|
|
||
| // TODO: Failing on invalid ReconcilePeriod is (I believe) new behavior |
There was a problem hiding this comment.
This is a change in behavior.
|
@jmrodri could you take a look at this? |
| // Run - A blocking function which starts a controller-runtime manager | ||
| // It starts an Operator by reading in the values in `./watches.yaml`, adds a controller | ||
| // to the manager, and finally running the manager. | ||
| func Run(done chan error, mgr manager.Manager, f *flags.AnsibleOperatorFlags, cMap *controllermap.ControllerMap) { | ||
| watches, err := runner.NewFromWatches(f.WatchesFile) | ||
| watchesSlice, err := watches.Load(f.WatchesFile) |
There was a problem hiding this comment.
@lilic was looking into adding support in the ansible operator for kube-state-metrics, which would require access to the GVKs from watches.yaml in pkg/ansible/run.go. The helm operator does the watch file parsing in pkg/helm/run.go and passes the necessary pieces into the controller and metrics registration functions.
Is something like that do-able for the ansible-operator case?
There was a problem hiding this comment.
I don't know enough about the impact of that sort of change so I'll just ping @shawn-hurley on that.
I do think that we could agree that the changes mentioned would be outside the scope of this PR? I would be happy to implement that move...and I would be happy to see this PR merged 😎
There was a problem hiding this comment.
yes I am happy if it's done in a separate PR, but it is something that needs to be done for the custom resource metrics to be enabled.
|
@joelanford you think that you could have a look at this PR? |
In the same way that Helm has a watches package responsible for loading the watches file, Ansible should be structured similiarly to separate concerns, primarily, loading and validating the watches and creating runners.
- Move the runner struct next to the functions that accept it (back to where it was before) - Remove fields from runner that aren't needed anymore since we load the watches first, we can put manageStatus, reconcilePeriod, watchDependentResources, watchClusterScopedResources on the controller
It is valid for a watch to have a finalizer with no role/playbook so long as it specifies vars to be run with the playbook/role specified on the watch.
|
New changes are detected. LGTM label has been removed. |
In the same way that Helm has a watches package responsible for loading
the watches file, Ansible should be structured similiarly to separate
concerns, primarily, loading and validating the watches and creating
runners.