Add more options to applications module #32
Conversation
- add managedNamespaceMetadata option in syncPolicy - add syncOption "CreateNamespace=true"
There was a problem hiding this comment.
I really appreciate the contribution and I like the addition of ignoreDifferences!
One thought, maybe it would be better to introduce it as an attrsOf instead of listOf that is then changed to a list in the Application (effectively applications.<name>.ignoreDifferences.<kind>, since kind is the only required option). This would allow you to influence an ignoreDifferences rule from different modules, which you can't easily do when it's just a list.
Regarding the addition of createNamespace in syncOptions, I knew this feature existed in ArgoCD but I decided to instead have nixidy support generating an actual Namespace manifest with applications.<name>.createNamespace because (and correct me if I'm wrong) ArgoCD doesn't clean up the namespace it creates with the CreateNamespace=true feature when the application is deleted. Is there any other benefits from having ArgoCD handle this?
I'm concerned with that it might be confusing having both.
Finally, since these are 2 kind of unrelated changes, could you split this into 2 PRs instead?
| config = { | ||
| "group" = mkOverride 1002 null; | ||
| "jqPathExpressions" = mkOverride 1002 null; | ||
| "jsonPointers" = mkOverride 1002 null; | ||
| "managedFieldsManagers" = mkOverride 1002 null; | ||
| "name" = mkOverride 1002 null; | ||
| "namespace" = mkOverride 1002 null; | ||
| }; |
There was a problem hiding this comment.
While the fromCRD generator sets defaults like this, I don't do it anywhere in the nixidy codebase. Could you instead set the default = null; in each respective mkOption instead?
| }); | ||
| inherit (app) ignoreDifferences; |
There was a problem hiding this comment.
A minor nitpick, could you merge this with inherit (app) project; at the top of spec.
|
Hey @adamchol, are you still interested in introducing these changes. I'm interested in the I could implement it myself but I want you to have the credit if it matters to you :) |
|
I am interested, sorry I couldn't find the time. Should be done today or tomorrow. |
No worries! Just checking :) |
|
@adamchol I'm closing this since we pulled the ignoreDifferences into another pull request. I'd like to still discuss the use of |
|
Hi! Actually, now I think there is no need for |
|
I have used this technique to add a label to a namespace created by Do you feel like abstracting this to a |
This PR adds those options to
applicationsmodule:ignoreDifferencesmanagedNamespaceMetadatainside thesyncPolicycreateNamespaceinside thesyncPolicy.syncOptionsThis changes were made mainly to match some values originally generated by an operator (especially the
createNamespacesync option which doesn't really make sense in normal cases). Seems like it's working fine with all the types. I didn't have time to write any tests for those options though and I'm not sure if this code fits well, so I'm open for any comments and I can do some cleanup later.