Skip to content

Conversation

@motoki317
Copy link

Summary

I have changed the params-cm default so users can use ApplicationSet's syncPolicy.applicationsSync override by default, without digging deep into the values.yaml defaults.
The issue is described in details at #3488.

closes #3488

Checklist:

  • I have bumped the chart version according to versioning
  • I have updated the documentation according to documentation
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.
  • I have created a separate pull request for each chart according to pull requests
  • My build is green (troubleshooting builds).

…er ApplicationSet by default

Signed-off-by: motoki317 <[email protected]>
@yu-croco
Copy link
Collaborator

yu-croco commented Sep 14, 2025

Hi @motoki317 , thank you for your PR. We follow upstream's manifests, which means we provide same default value as upstream. If you want to change the default value, could you please approach to upstream at first?
FYI; though current values.yaml doesn't provide the interface for applicationsetcontroller.enable.policy.override, you can override in your values.yaml.

@motoki317
Copy link
Author

Hi @motoki317 , thank you for your PR. We follow upstream's manifests, which means we provide same default value as upstream. If you want to change the default value, could you please approach to upstream at first?

Hi, thanks for the reply.
By upstream you mean the install.yaml at the releases? As far as I know, the upstream cmd-params-cm doesn't contain any values. But this chart contains many "default" values.
It is exactly these non-default values causing problem as described in #3488. So my proposed solution is to either add this new default value in valus.yaml, or remove applicationsetcontroller.policy altogether, to align with the upstream install.yaml behavior.
The current chart behavior does not align with the upstream install.yaml as far as I know.

@motoki317
Copy link
Author

Hi, what do you think about this?
Although this change itself is titled "change default", it's more about a fix for the current chart bug.
The current chart does not align with the upstream behavior and doesn't seem right.

@yu-croco
Copy link
Collaborator

yu-croco commented Sep 17, 2025

So my proposed solution is to either add this new default value in valus.yaml, or remove applicationsetcontroller.policy altogether, to align with the upstream install.yaml behavior.

I agree with your latter suggestion. It's best to align with the upstream default, which is currently an empty string "", as shown here.
It seems that the default value of applicationsetcontroller.policy was originally sync when it was released in v2.6.0 and we inherited it in this PR correctly. We don't have enough capability to follow the specific values of cmd-params and users can override it by themselves, so I think removing it makes sense.

WDYT? 👀 @mbevc1 @mkilchhofer @jmeridth @pdrastil @tico24

@motoki317
Copy link
Author

Thanks, I see this was resolved in #3540.

@motoki317 motoki317 closed this Oct 18, 2025
@motoki317 motoki317 deleted the feat/argo-cd-appset-policy-override branch October 18, 2025 01:33
@yu-croco
Copy link
Collaborator

FYI; we removed default values for .Values.configs.params.xxxx in #3540 but changelog in README was wrong. If you want to update to v9.0.x, please also check v9.0.2 ( #3546 ) in addition to v9.0.0. 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[argo-cd] Add applicationsetcontroller.enable.policy.override to params-cm by default?

2 participants