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: refactor resources/data to use the new serialization/deserializ… #413

Merged
merged 5 commits into from
Jun 13, 2022

Conversation

TomerHeber
Copy link
Collaborator

Solution

Some refactoring in resource configuration variable.
Part of a larger refactoring effort #376

return diag.Errorf(err.Error())
}

var type_ client.ConfigurationVariableType
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The serializer knows how to handle this use case.
see environment.go func (c *ConfigurationVariableType) ReadResourceData(fieldName string, d *schema.ResourceData) error {...

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.

looks really good. I just need one clarification about some checks that have no handling with the negative outcome

env0/utils.go Show resolved Hide resolved
assert.Equal(t, params.Description, "description")
}

func TestReadByPointerCustomResourceData(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.

additional test for the use-case.

assert.Equal(t, params.Description, "description")
}

func TestReadByPointerNilCustomResourceData(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.

I want to be sure if not set in schema, params.Type remains nil.

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.

LGTM. After the tests will pass, merge 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 Jun 13, 2022
@TomerHeber TomerHeber merged commit 369c1ca into main Jun 13, 2022
@TomerHeber TomerHeber deleted the feat-refactor-serialization-#376 branch June 13, 2022 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

2 participants