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

Bug: environment resource configuration field is affected by global s… #326

Merged
merged 2 commits into from
Apr 11, 2022

Conversation

TomerHeber
Copy link
Collaborator

Environment resource configuration field is affected by global scope variables

Issue & Steps to Reproduce / Feature Request

fixes #318

Solution

In the environment resource: filter out any configuration that is not within the environment scope.

@TomerHeber TomerHeber requested a review from yaronya April 7, 2022 20:44
@RLRabinowitz
Copy link
Contributor

Couple of questions:

  1. How did this code change change the behaviour here? It's a bit unclear to me
  2. Maybe instead of filtering out the scope, let the API only ask for variables in environment scope? As-in - have the API not send the organizationId on its request

@RLRabinowitz
Copy link
Contributor

We talked. I was wrong about the API, it must receive organizationId.
We're also not sure how this code used to work correctly in 0.2.31.

For now, we'll go with this approach

@TomerHeber as discussed, this only needs some tests

@TomerHeber TomerHeber force-pushed the bug-environment-resource-global-scope-#318 branch from f564034 to 6209d6a Compare April 10, 2022 19:42
@TomerHeber TomerHeber force-pushed the bug-environment-resource-global-scope-#318 branch 2 times, most recently from 68938a7 to 9b4dc34 Compare April 10, 2022 19:48
@TomerHeber TomerHeber force-pushed the bug-environment-resource-global-scope-#318 branch from 9b4dc34 to 0f0b2ba Compare April 10, 2022 20:06
@@ -100,6 +100,18 @@ func (client *ApiClient) ConfigurationVariablesByScope(scope Scope, scopeId stri
if err != nil {
return []ConfigurationVariable{}, err
}

if scope != ScopeGlobal {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@RLRabinowitz - decided to fix the issue here. This covers much more use-cases.
I hope it makes sense: I filter out the global scope if the requested scope isn't global.

@@ -247,7 +263,7 @@ var _ = Describe("Configuration Variable", func() {

Describe("ConfigurationVariablesByScope", func() {
var returnedVariables []ConfigurationVariable
mockVariables := []ConfigurationVariable{mockConfigurationVariable}
mockVariables := []ConfigurationVariable{mockConfigurationVariable, mockGlobalConfigurationVariable}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As discussed. Added the use-case to the unit tests.

@TomerHeber
Copy link
Collaborator Author

Note: I was unable to get the acceptance test working due to timeout.
See: https://env0.slack.com/archives/C01QBBEGBEH/p1649617036221799

Copy link
Contributor

@RLRabinowitz RLRabinowitz 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 👍🏻

@github-actions github-actions bot added ready to merge PR approved - can be merged once the PR owner is ready and removed pending final review labels Apr 11, 2022
@TomerHeber TomerHeber merged commit d58ecd8 into main Apr 11, 2022
@TomerHeber TomerHeber deleted the bug-environment-resource-global-scope-#318 branch April 11, 2022 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-client 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.

Environment resource configuration field is affected by global scope variables
2 participants