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

Feat: add support for agent settings #330

Merged
merged 3 commits into from
Apr 13, 2022

Conversation

TomerHeber
Copy link
Collaborator

Issue & Steps to Reproduce / Feature Request

resolves #316

Solution

Added API call.
Added data-agents source.
Added unit tests.
Added documentation.

@@ -0,0 +1,11 @@
resource "env0_project" "example" {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note/Question:

Is there a way to test this in harness?
I don't believe I have a way to create agents. Open to suggestions.

At the moment there are no added integration tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

That wouldn't be ROI positive at the moment so I'd leave it without harness tests

Copy link
Contributor

@yaronya yaronya left a comment

Choose a reason for hiding this comment

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

Left one comment

}

// Not really needed. But required by Terraform SDK - https://github.com/hashicorp/terraform-plugin-sdk/issues/541
d.SetId("1")
Copy link
Contributor

Choose a reason for hiding this comment

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

Does that mean that a user cannot have more than a single data resource of agents in his TF code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yaronya ,

I can make it random if preferred.
However, why would using "1" as the ID prevent it from being used in multiple places?

See this bug fix in the google provider:
https://github.com/hashicorp/terraform-provider-google/pull/7375/files

Copy link
Contributor

Choose a reason for hiding this comment

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

However, why would using "1" as the ID prevent it from being used in multiple places?

I think that the id has to be unique across the TF state, isn't it?
Random is also not so great, but preferable than "1".
I'd try and see if we can get from the schema/context the resource name and set it based on it (since it has to be unique anyways on your TF stack).
WDYT?

See this bug fix in the google provider

The example you just sent has the id based on some dynamic parameters such as project id and zone id

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yaronya - regardless wouldn't it potentially cause duplication of ID (the example I sent).

Copy link
Contributor

Choose a reason for hiding this comment

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

@yaronya - regardless wouldn't it potentially cause duplication of ID (the example I sent).

@TomerHeber
Yes, I believe it may create such duplication, but that example has somewhat lower chance to do so as it depends on at least some parameters
I'm assuming it'd break stuff as the id has to be unique, but maybe I'm wrong.

@@ -0,0 +1,11 @@
resource "env0_project" "example" {
Copy link
Contributor

Choose a reason for hiding this comment

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

That wouldn't be ROI positive at the moment so I'd leave it without harness tests

@github-actions github-actions bot added the ready to merge PR approved - can be merged once the PR owner is ready label Apr 13, 2022
@TomerHeber TomerHeber merged commit 6c384ce into main Apr 13, 2022
@TomerHeber TomerHeber deleted the feat-add-support-for-agent-settings-#316 branch April 13, 2022 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-client examples feature provider ready to merge PR approved - can be merged once the PR owner is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for agent settings
2 participants