Closed JustinKuli closed 1 year ago
Kudos, SonarCloud Quality Gate passed!
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: JustinKuli, willkutler
The full list of commands accepted by this bot can be found here.
The pull request process is described here
looks good!
oops, I didn't realize there wasn't a /hold
label set. @dhaiducek are you also reviewing this? I can revert if you have comments
No worries--I'll take another look tomorrow, but quick look looks good to me! I especially like the orderViaDependencies
to generate dependencies automatically!
I suppose the only thing I'm wondering about is whether orderViaDependencies
should be moved to policyDefaults
so that it can be specified at the individual policy level. It'd add another layer of complexity but might be nice to, for example, deploy an operator policy with all the operator things having dependencies and then have everything else not have dependencies?
~Actually, the extraDependencies
field is missing here. Are there future plans to add it?~
~Actually, the
extraDependencies
field is missing here. Are there future plans to add it?~
Oops, should have read the description first! I agree it could be complex, but if we're not implementing it today, maybe open an enhancement issue for it? I'm thinking there could potentially be a use-case for it.
Looking at this with fresher eyes, I'm confused by some of my choices - which might not be a good sign!
I think I incorrectly doc'd orderViaDependencies
: it says "order they are defined in the manifests list" but actually it orders the output policies based on their order in the policies list. With that in mind, it could be useful to have a similar option in policyDefaults
/ individual policies to use extraDependencies
to order the ConfigurationPolicies based on their order in the manifests list.
Maybe it looked hard when I was in the code implementing it, but now it definitely seems like adding ignorePending
and extraDependencies
options to the manifest options would be good.
@dhaiducek
I suppose the only thing I'm wondering about is whether orderViaDependencies should be moved to policyDefaults so that it can be specified at the individual policy level. It'd add another layer of complexity but might be nice to, for example, deploy an operator policy with all the operator things having dependencies and then have everything else not have dependencies?
Setting orderViaDependencies
on individual policies seems hard to reason through: if I have 5 policies, and I set it on all except for the third, do I want a dependency graph like 1->2, 3->4->5
or 1->2, 3, 4->5
or 1->2->4->5, 3
? To prevent ambiguity, I'd rather just have it be all-or-nothing. If a user wants a policy outside of the chain, or working some other way, they can put it in a separate generator file!
Setting
orderViaDependencies
on individual policies seems hard to reason through: if I have 5 policies, and I set it on all except for the third, do I want a dependency graph like1->2, 3->4->5
or1->2, 3, 4->5
or1->2->4->5, 3
? To prevent ambiguity, I'd rather just have it be all-or-nothing. If a user wants a policy outside of the chain, or working some other way, they can put it in a separate generator file!
Maybe what I'm saying doesn't make sense. I'm thinking something driven by consolidateManifests
where multiple ConfigurationPolicies are generated (one for each manifest) and ordering would make sense:
policyDefaults:
orderViaDependencies: <-- Add dependencies for Policies; If `consolidateManifests` is false and there are multiple manifests, add extraDependencies for each ConfigurationPolicy within each Policy
policies:
- orderViaDependencies: <-- Override the policyDefaults setting for this policy
manifests:
- orderViaDependencies: <-- Override the policies setting for these manifests (but only if `consolidateManifests` is false where specifying the `extraDependencies` would make sense and there are multiple manifests)
Oh wait. I see what you're saying now. What about not allowing overrides at policies[].orderViaDependencies
but allowing it within the policy for extraDependencies
?
...or like you said (now that I'm reading it again), having an additional setting for the extraDependencies
ordering, like orderViaExtraDependencies
or something.
The new
dependencies
field in the generator is the same as the newdependencies
field in the Policy type, however, more fields are optional and will be filled in by the generator with useful defaults.The new
ignorePending
field is also the same, but since that would be set on templates, and the generator does not expose much at the template level, here the field is set on the policy, and the generator sets the correct field in all the templates of that policy. For a similar reason, theextraDependencies
field is not exposed in the generator at all.The new
orderViaDependencies
field will set up a chain of dependencies on the created policies. So if 3 policies are created by the generator, policy 2 will depend on policy 1, and policy 3 will depend on policy 2.In all cases, dependency lists are merged - setting it on a specific policy will not override the defaults, or a dependency added by the
orderViaDependencies
flag.Refs:
Signed-off-by: Justin Kulikauskas jkulikau@redhat.com