Add applications sync to application set sync policies#337
Conversation
…ies' of github.com:ahublersos/terraform-provider-argocd into 333_add_applications_sync_to_application_set_sync_policies
|
I don't know why the |
Yeah, that's just a flaky test. Everything passing now. Will try take a look at the changes a some point today/tomorrow morning. |
onematchfox
left a comment
onematchfox
left a comment
There was a problem hiding this comment.
Sorry for the slow feedback. Hopefully just one small change - otherwise LGTM.
| ) | ||
|
|
||
| func expandApplicationSet(d *schema.ResourceData, featureMultipleApplicationSourcesSupported bool) (metadata meta.ObjectMeta, spec application.ApplicationSetSpec, err error) { | ||
| func expandApplicationSet(d *schema.ResourceData, featureMultipleApplicationSourcesSupported bool, featureApplicationSetApplicationsSyncPolicy bool) (metadata meta.ObjectMeta, spec application.ApplicationSetSpec, err error) { |
There was a problem hiding this comment.
I realise this is a bit confusing, but can you please drop this parameter (and it's usage in this file) and rather use the feature flag to implement a check on the user's provided configuration as per https://github.com/oboukili/terraform-provider-argocd/blob/d84d09101f8c1954cf613df69890a846540692b7/argocd/resource_argocd_application_set.go#L49-L51.
In essence, this is the pattern used by all other feature flags - we check the user's configuration of the Terraform resource and return an error if they try to configure something via Terraform that isn't supported by their version of ArgoCD (thereby preventing the configuration from being applied to the resulting CRUD call the provider makes). Multiple applications sources is the exception here in that we support versions that both have and do not have the feature and thus, need branching logic in what we set on the CRUD call.
There was a problem hiding this comment.
Got it, thanks for the explanation! I've removed this parameter and added a feature flag check in the resource.
|
This pr is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
github-actions
Bot
added
the
Stale
|
This isn't stale. I just need to get around to looking at the failing tests. |
github-actions
Bot
removed
the
Stale
onematchfox
changed the title
|
Sorry for the delay here @ahublersos and thanks for your patience! |
|
@ahublersos can/should I go ahead and cut a new release now? |
|
@onematchfox yes, please do! |
Sorry... more delays. Our release pipeline is broken. Will try upgrade/fix this weekend. |
2 participants
Fixes #333.
Adds support for applications_sync to the application set resource.
I also nearly installed the test manifests into a live cluster by running
testacc_prepare_envso I added a few safety safety nets to prevent that from happening in the future. Happy to split this out into a new PR if that's preferred.