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

fest: Environment Resource - configuration variables read & delete #182

Merged
merged 7 commits into from
Dec 16, 2021

Conversation

GiliFaroEnv0
Copy link
Contributor

@GiliFaroEnv0 GiliFaroEnv0 commented Dec 13, 2021

Issue & Steps to Reproduce / Feature Request

Environment Resource - configuration variables read & delete

Solution

Added Read logic for configuration variables
Added delete and update logic for configuration variables

@GiliFaroEnv0
Copy link
Contributor Author

Copy link
Member

@eranelbaz eranelbaz left a comment

Choose a reason for hiding this comment

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

Looks good
please also add this use case to the integration tests
and generate the docs

Also your CI is failing

d.SetId(environment.Id)
setEnvironmentSchema(d, environment)
setEnvironmentSchema(d, environment, client.ConfigurationChanges{})
Copy link
Member

Choose a reason for hiding this comment

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

Cursed language
Why no optional parameters?
damn you go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hate this
also no Ternary Operator ?:

as for the integration tests will be added on a different ticket to unblock the QA PARTY 🎉

Copy link
Member

Choose a reason for hiding this comment

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

Thats not a blocker, as those are terraform files we need to update

Copy link
Member

Choose a reason for hiding this comment

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

update this file
https://github.com/env0/terraform-provider-env0/blob/main/tests/integration/012_environment/main.tf
add the configuration part for the resource to create it and also data to read it
then expose a output and you need to assert the output is as you expect using this file
https://github.com/env0/terraform-provider-env0/blob/main/tests/integration/012_environment/expected_outputs.json

really small part

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Why not here?
thats really small change

Copy link
Contributor Author

@GiliFaroEnv0 GiliFaroEnv0 Dec 15, 2021

Choose a reason for hiding this comment

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

@eranelbaz its not tried alot but its not
since on deploying the scope of the configuration variable changes to DEPLOYMENT and we can’t search by this scope

Copy link
Member

Choose a reason for hiding this comment

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

lets work this out in a next pr

env0/resource_environment.go Outdated Show resolved Hide resolved
@GiliFaroEnv0 GiliFaroEnv0 force-pushed the feat-configure-variables-read-and-delete branch from 7379180 to 4dc342f Compare December 15, 2021 15:30
@eranelbaz eranelbaz merged commit 6034026 into main Dec 16, 2021
@eranelbaz eranelbaz deleted the feat-configure-variables-read-and-delete branch December 16, 2021 08:49
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.

None yet

2 participants