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

Fix: configuration variable that overrides a template variable in env… #955

Merged
merged 2 commits into from
Sep 25, 2024

Conversation

TomerHeber
Copy link
Collaborator

…0_environment shows drift

Part of #894

Solution

Since the change is large and complex, this PR is the first part of fixing the issue.
For now I've just added the Overwrites field to the struct.

IsReadOnly *bool `json:"isReadonly,omitempty"`
IsRequired *bool `json:"isRequired,omitempty"`
Regex string `json:"regex,omitempty"`
Overwrites *ConfigurationVariableOverwrites `json:"overwrites,omitempty"` // Is removed when marhseling to a JSON.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This struct is also sent in a POST requests.
Overwrites field should not be sent in a request. (We are only intrested of this value in a response).
To solve this issue without making too many code changes, I have decided to write a custom marshal.

v.Overwrites = nil

// This is done to prevent an infinite loop.
type ConfigurationVariableDummy ConfigurationVariable
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

without doing the following an infinite loop will happen, because inside marshal we are calling marshal again on the same object.

@@ -77,6 +86,17 @@ type ConfigurationVariableUpdateParams struct {
Id string
}

func (v ConfigurationVariable) MarshalJSON() ([]byte, error) {
v.Overwrites = nil
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

avoid sending Overwrites when marshaling a json to bytes.


dummy := ConfigurationVariableDummy(v)

return json.Marshal(&dummy)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this would have caused loops...

@@ -291,3 +295,37 @@ var _ = Describe("Configuration Variable", func() {
})
})
})

func TestConfigurationVariableMarshelling(t *testing.T) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this test was added to verify that the marshal and unmarshal are working as expected.

@@ -0,0 +1,143 @@
package env0
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved these functions to here from env0/resource_environment.go
(These are mostly commom functions).

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 changes for now...

@yaronya
Copy link
Contributor

yaronya commented Sep 25, 2024

/review

@bot-env0 bot-env0 requested a review from a team September 25, 2024 05:50
Copy link
Contributor

@sagilaufer1992 sagilaufer1992 left a comment

Choose a reason for hiding this comment

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

BeforeEach(func() {
httpCall = mockHttpClient.EXPECT().Delete("configuration/"+mockConfigurationVariable.Id, nil)
apiClient.ConfigurationVariableDelete(mockConfigurationVariable.Id)
_ = apiClient.ConfigurationVariableDelete(mockConfigurationVariable.Id)
Copy link
Contributor

Choose a reason for hiding this comment

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

why these changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

using a new linter - fixes a linting issue.
It tells the linter, I know that this can return an error - but I want to ignore it.

@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 Sep 25, 2024
@TomerHeber TomerHeber merged commit 3bd6be6 into main Sep 25, 2024
3 of 4 checks passed
@TomerHeber TomerHeber deleted the fix-configuration-override-#894 branch September 25, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-client fix 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.

3 participants