Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add: project scope support for clusters #143

Merged
merged 2 commits into from
Feb 25, 2022

Conversation

alxbse
Copy link
Contributor

@alxbse alxbse commented Feb 24, 2022

Adds support for project scoped clusters: https://argo-cd.readthedocs.io/en/stable/user-guide/projects/#project-scoped-repositories-and-clusters

I'm deliberately adding an acceptance tests that breaks with 2.1.0 because that version does not have support for scoped clusters, any ideas on how to deal with tests that require at least some version of argocd? I'm thinking if adding a precheck function to check the argocd version and skip if it is not compatible could be a good solution?

Copy link
Collaborator

@oboukili oboukili left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question about the precheck, easy way would be to ignore silently the project attribute inside the expansion function if the version is below the constraint (and log a debug comment).
There is a map in features.go that may help you do that.

@@ -121,6 +121,7 @@ resource "argocd_cluster" "eks" {
* `namespaces` - (Optional) Holds list of namespaces which are accessible in that cluster. Cluster level resources would be ignored if namespace list is not empty..
* `config` - (Optional) The configuration specification, nested attributes are documented below.
* `metadata` - (Optional) Cluster metadata, nested attributes are documented below.
* `project` - (Optional) Scope cluster to ArgoCD project. If omitted, cluster will be global.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a version requirement comment here, and what happens if the field is added on argocd versions < 2.2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2.2.0 being required has been added. Also, based on how the application resource does it I added a new feature to the constraints map so if the setting is used with an older version it errors out.

Useful for acceptance tests that should only be run when specific
features are supported by ArgoCD version.

If a feature is not supported the test is skipped, not failed.
@alxbse alxbse force-pushed the add-project-to-cluster branch from c59807e to 9f7503e Compare February 24, 2022 20:56
@alxbse alxbse force-pushed the add-project-to-cluster branch from 9f7503e to 0148af3 Compare February 24, 2022 21:00
@alxbse
Copy link
Contributor Author

alxbse commented Feb 24, 2022

I was looking around in other terraform providers if there are any establish ways of doing prechecks that check server versions but have not been able to find any yet.

I added a slightly hackish precheck function that uses the constraints map to compare what version is required for the acceptance test to run. It uses the ARGOCD_VERSION environment variable to figure out what version it is running against, which is not the nicest solution.

Would it be possible to access the actual provider and the initialized ServerInterface from any function? It would be nicer to get the actual version from the server using the isFeatureSupported function.

@alxbse
Copy link
Contributor Author

alxbse commented Feb 25, 2022

Looking at how github.com/hashicorp/terraform-provider-vsphere does it I added an alternative implementation of the precheck function in alxbse/terraform-provider-argocd@2e5f71d. It is still kinda hacky but atleast it uses the same isFeatureSupported function call.

It feels like I'm get further away from the actual issue of adding support for project scope, should I maybe close this PR for now to sort out prechecking in another one?

@oboukili
Copy link
Collaborator

As you mentioned, we would need to establish another ServerInterface just for the tests. Your solution is acceptable, and the impact on local testing is minimal. For now I think that will do, we can indeed tackle this issue in another PR.

@oboukili oboukili merged commit 647debc into argoproj-labs:master Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants