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: do not send terraformVersion unused field #701

Merged
merged 3 commits into from
Aug 29, 2023

Conversation

TomerHeber
Copy link
Collaborator

Issue & Steps to Reproduce / Feature Request

fixes #659

Solution

  1. added omitempty and emptied terraform version if the template type is not "terraform" or "terragrunt".
  2. tests updated accordingly.

@@ -78,7 +78,7 @@ type TemplateCreatePayload struct {
GitlabProjectId int `json:"gitlabProjectId,omitempty"`
Revision string `json:"revision"`
OrganizationId string `json:"organizationId"`
TerraformVersion string `json:"terraformVersion"`
TerraformVersion string `json:"terraformVersion,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @TomerHeber , I'm Sagy from RND, nice to github-meet you 😃
on purpose you don't have the tfschema:",omitempty" syntax here also?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @sagydr - nice to meet you (:

tfschema omitempty is only needed when reading from a struct and writing to the terraform schema.
For this struct we read from the terraform schema and write to the struct.

When transforming to a json, the json omitempty will be used and the field will not be sent.
(if string == "" then the field isn't passed in the json).

Happy to explain more if this is still unclear. (Didn't want the explanation to be too long).

@@ -111,7 +111,7 @@ type VariablesFromRepositoryPayload struct {
Repository string `json:"repository"`
}

func (payload TemplateCreatePayload) Validate() error {
func (payload *TemplateCreatePayload) Invalidate() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the asterix * is used to de-reference the variable to its value. Did it also work before this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @sagydr - ahh that's a golang question (:
without "" is readonly function - you can't modify "this".
with "
" you can modify "this".

Comment on lines +630 to +631
templateCreatePayload.TerraformVersion = ""
updateTemplateCreateTemplate.TerraformVersion = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity as I'm not too familiar with Go, templateCreatePayload & updateTemplateCreateTemplate have many common fields, is it not common to extract them to something like BaseTemplate struct? Like in Python?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can be done, but not very common.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To add on this: Golang does not have inheritance and is not object oriented.
https://go.dev/doc/faq#:~:text=Types-,Is%20Go%20an%20object%2Doriented%20language%3F,in%20some%20ways%20more%20general.

Copy link
Contributor

@sagydr sagydr left a comment

Choose a reason for hiding this comment

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

Added some questions, but code looks straight-forward

@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 Aug 29, 2023
@TomerHeber TomerHeber merged commit d7b2e68 into main Aug 29, 2023
6 checks passed
@TomerHeber TomerHeber deleted the fix-do-not-send-tf-ver-#659 branch August 29, 2023 15:57
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.

Do not send terraformVersion unused field
2 participants