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.

Sign up for GitHub

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

Jump to bottom

Fix: drift issues with variable sets #889

Merged
merged 2 commits into from
Jul 11, 2024
Merged

Fix: drift issues with variable sets #889

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
581a96a
Fix: drift issues with variable sets
TomerHeber Jul 8, 2024
675c46e
don't ignore drift
TomerHeber Jul 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions client/configuration_set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@ type UpdateConfigurationSetPayload struct {
}

type ConfigurationSet struct {
Id string `json:"id"`
Name string `json:"name"`
Description string `json:"description"`
Id string `json:"id"`
Name string `json:"name"`
Description string `json:"description"`
AssignmentScope string `json:"assignmentScope"`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added assignment scope to be able to filter implicit assignments.

}

func (client *ApiClient) ConfigurationSetCreate(payload *CreateConfigurationSetPayload) (*ConfigurationSet, error) {
Expand Down
8 changes: 5 additions & 3 deletions env0/resource_environment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ func resourceEnvironment() *schema.Resource {
},
"variable_sets": {
Type: schema.TypeList,
Description: "a list of variable set to assign to this environment",
Description: "a list of variable set to assign to this environment. Note: must not be used with 'env0_variable_set_assignment'",
Optional: true,
Elem: &schema.Schema{
Type: schema.TypeString,
Expand Down Expand Up @@ -433,7 +433,7 @@ func setEnvironmentSchema(ctx context.Context, d *schema.ResourceData, environme

setEnvironmentConfigurationSchema(ctx, d, configurationVariables)

if d.Get("variable_sets") != nil {
if variableSets, ok := d.GetOk("variable_sets"); variableSets != nil && ok {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same as "configuration" - if this field isn't used - ignore any use-cases related to it.
this will avoid possible conflicts with explicit assignment resource.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if it's a basic question but I'm pretty new to go and terraform provider so I would like to make sure 😅 Let's say we have an environment and we assign a variable set to it (by FE or API) will we see the drift there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need to apologize (:
Correct - without this check we will see a drift.
(We are doing the same check for "configuration").

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, I talked about it with @GiliFaroEnv0 and it looked weird that we won't show DD but if that's how we do it for configuration it might be the way 🤔
I will raise it with the team and update you 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

we can't hide drifts!
the set assignment might be from the FE and not from the other resource
so this is really a drift

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we are not hiding drifts...
you can either use the assignment resource or set the sets inline in the environment.
You cannot use both. Otherwise, you will have constant drifts/conflicts. (Same with configuration).

It just tests which one is used... if a resource assignments are used, it ignores anything inline in the environment.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we just don't understand the code...
how are you checking if another resource is used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's simple (but not perfect):

if "variables-set" is set in the state, it means we use the "variables-set" inline use-case.
Otherwise, it's ignored.

if err := d.Set("variable_sets", variableSetsIds); err != nil {
return fmt.Errorf("failed to set variable_sets value: %w", err)
}
Expand Down Expand Up @@ -630,7 +630,9 @@ func getEnvironmentVariableSetIdsFromApi(d *schema.ResourceData, apiClient clien

var environmentVariableSetIds []string
for _, variableSet := range environmentVariableSets {
environmentVariableSetIds = append(environmentVariableSetIds, variableSet.Id)
if variableSet.AssignmentScope == "ENVIRONMENT" {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

filter is required here... to avoid getting upper scopes variables (E.g. project or org).

environmentVariableSetIds = append(environmentVariableSetIds, variableSet.Id)
}
}

return environmentVariableSetIds, nil
Expand Down
12 changes: 8 additions & 4 deletions env0/resource_environment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,19 +261,23 @@ func TestUnitEnvironmentResource(t *testing.T) {

configurationSets := []client.ConfigurationSet{
{
Id: "id1",
Id: "id1",
AssignmentScope: "ENVIRONMENT",
},
{
Id: "id2",
Id: "id2",
AssignmentScope: "ENVIRONMENT",
},
}

updatedConfigurationSets := []client.ConfigurationSet{
{
Id: "id3",
Id: "id3",
AssignmentScope: "ENVIRONMENT",
},
{
Id: "id2",
Id: "id2",
AssignmentScope: "ENVIRONMENT",
},
}

Expand Down
10 changes: 10 additions & 0 deletions env0/resource_variable_set_assignment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ func resourceVariableSetAssignment() *schema.Resource {
ReadContext: resourceVariableSetAssignmentRead,
DeleteContext: resourceVariableSetAssignmentDelete,

Description: "note: avoid assigning to environments using 'variable_sets' (see environment schema).",

Schema: map[string]*schema.Schema{
"scope": {
Type: schema.TypeString,
Expand Down Expand Up @@ -87,6 +89,10 @@ func resourceVariableSetAssignmentUpdate(ctx context.Context, d *schema.Resource

// In API but not in Schema - delete.
for _, apiConfigurationSet := range apiConfigurationSets {
if apiConfigurationSet.AssignmentScope != assignmentSchema.Scope {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

filter to avoid getting implicit variable sets... (from other scopes)

continue
}

found := false

apiSetId := apiConfigurationSet.Id
Expand Down Expand Up @@ -181,6 +187,10 @@ func resourceVariableSetAssignmentRead(ctx context.Context, d *schema.ResourceDa
}

for _, apiConfigurationSet := range apiConfigurationSets {
if apiConfigurationSet.AssignmentScope != assignmentSchema.Scope {
continue
Copy link
Collaborator Author

@TomerHeber TomerHeber Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

filter to avoid getting implicit variable sets... (from other scopes)

}

apiSetId := apiConfigurationSet.Id
found := false

Expand Down
24 changes: 16 additions & 8 deletions env0/resource_variable_set_assignment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,29 +20,35 @@ func TestUnitVariableSetAssignmentResource(t *testing.T) {
setIds := []string{"a1", "a2"}
configurationSetIds := []client.ConfigurationSet{
{
Id: "a1",
Id: "a1",
AssignmentScope: scope,
},
{
Id: "a2",
Id: "a2",
AssignmentScope: scope,
},
}
// Validates that drifts do not occur due to ordering.
flippedConfigurationSetIds := []client.ConfigurationSet{
{
Id: "a2",
Id: "a2",
AssignmentScope: scope,
},
{
Id: "a1",
Id: "a1",
AssignmentScope: scope,
},
}

updatedSetIds := []string{"a1", "a3"}
updatedConfigurationSetIds := []client.ConfigurationSet{
{
Id: "a3",
Id: "a3",
AssignmentScope: scope,
},
{
Id: "a1",
Id: "a1",
AssignmentScope: scope,
},
}

Expand Down Expand Up @@ -97,10 +103,12 @@ func TestUnitVariableSetAssignmentResource(t *testing.T) {
setIds := []string{"a1"}
configurationSetIds := []client.ConfigurationSet{
{
Id: "a1",
Id: "a1",
AssignmentScope: scope,
},
{
Id: "a2",
Id: "a2",
AssignmentScope: scope,
},
}

Expand Down
Loading