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 ignored json meta tags for run variables in RunCreateOptions #531

Merged
merged 1 commit into from Sep 27, 2022

Conversation

Uk1288
Copy link
Contributor

@Uk1288 Uk1288 commented Sep 13, 2022

Description:

**NOTE: TheVariables field in Run has been changed from type []*RunVariable to []*RunVariableAttr **

Ref PR: #204

Run variables in RunCreateOptions are not marshalled in line with the API (https://www.terraform.io/cloud-docs/api-docs/run#request-body). The variables are currently marshalled as:

"variables": [
        {
          "Key": "aKey",
          "Value": "\"aValue\""
        }
]

instead of

"variables": [
        {
          "key": "aKey",
          "value": "\"aValue\""
        }
]

This PR adds changes that marshals and un-marshals the run variables correctly.

Testing plan

  1. Create a run with options
	newRun, err := client.Runs.Create(context.Background(), tfe.RunCreateOptions{
		Workspace: &tfe.Workspace{
			ID: "ws-ID",
		},
		Variables: []*tfe.RunVariableOption{
			{
				Key:   "name",
				Value: "\"aName\"",
			},
		},
	})
  1. Run should be created with correct variable key, name attributes,
  2. Run value in go client should have Key, Value fields populated as well.

External links

Output from tests

Including output from tests may require access to a TFE instance. Ignore this section if you have no environment to test against.

$ TFE_ADDRESS="https://example" TFE_TOKEN="example" TF_ACC="1" go test ./... -v -tags=integration -run TestRunCreateOptions_Marshal

=== RUN   TestRunCreateOptions_Marshal
--- PASS: TestRunCreateOptions_Marshal (1.47s)
PASS
ok 

@Uk1288 Uk1288 requested a review from a team as a code owner September 13, 2022 18:56
run.go Outdated
Comment on lines 332 to 336
type RunVariableOption struct {
Key string `json:"key"`
Value string `json:"value"`
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it bet better to just redefine the RunVariable struct to be decorated as json: instead of jsonapi: ? I think that was the intent and it just got decorated incorrectly. By me, probably!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially tried redefining the RunVariable, in that case it will marshal correctly from Go value to json but un-marshalling from json to Go value stops working. The only way I found to make both direction work is if I create the two distinct types.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I see what you mean, now. Since RunVariable was used as a create option, which more often times requires callers to specify the exact type name (example), I think it would be better to redefine that as "json:" and create a new type for use in the Run struct. Like this WDYT?

I think then we could get away with not reving the major version... Although we should still note this type change in the release notes in the context of a bug fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this makes sense. I have renamed attributes to RunVariable and RunVariableAttr. This way all existing Create run calls will still work.

Copy link
Contributor

@annawinkler annawinkler left a comment

Choose a reason for hiding this comment

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

If this is a breaking change, is this a major version bump then?
cc @brandonc

@brandonc
Copy link
Collaborator

@annawinkler It's not a breaking change because the behavior doesn't match the documentation. (It capitalizes the keys in the request, which technically works but it's not expected) So this is just a bug fix.

@brandonc
Copy link
Collaborator

@annawinkler I see now that you mean modifying the actual type. Let me think about it. It would probably be less impactful to add a new output type and redefine the existing input type.

@Uk1288 Uk1288 force-pushed the uk1288-fix-run-variable-meta-tags branch 2 times, most recently from 8498b0a to 99fe1c4 Compare September 14, 2022 20:07
brandonc
brandonc previously approved these changes Sep 15, 2022
Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

Don't forget to add CHANGELOG

@laurenolivia
Copy link
Contributor

Hmm looks like StateVersion is flaking on your PR 🤔 But I checked out this branch and you have the latest changes.

@laurenolivia
Copy link
Contributor

Testing plan works as expected! ✅

CHANGELOG.md Outdated
@@ -2,6 +2,7 @@

## Bug Fixes
* Fixes null value returned in variable set relationship in `VariableSetVariable` by @sebasslash [#521](https://github.com/hashicorp/go-tfe/pull/521)
* Fix marshalling of run variables in `RunCreateOptions` by @Uk1288 [#531](https://github.com/hashicorp/go-tfe/pull/531)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's mention the type that changed as well.

@Uk1288 Uk1288 force-pushed the uk1288-fix-run-variable-meta-tags branch from 4a746b1 to 956527b Compare September 19, 2022 18:53
@@ -2,6 +2,7 @@

## Bug Fixes
* Fixes null value returned in variable set relationship in `VariableSetVariable` by @sebasslash [#521](https://github.com/hashicorp/go-tfe/pull/521)
* Fix marshalling of run variables in `RunCreateOptions`. The `Variables` field type in `Run` struct has changed from `[]*RunVariable` to `[]*RunVariableAttr` by @Uk1288 [#531](https://github.com/hashicorp/go-tfe/pull/531)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! 🌟

Copy link
Contributor

@annawinkler annawinkler left a comment

Choose a reason for hiding this comment

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

:shipit:

@Uk1288 Uk1288 merged commit ff71068 into main Sep 27, 2022
@Uk1288 Uk1288 deleted the uk1288-fix-run-variable-meta-tags branch September 27, 2022 20:01
@github-actions
Copy link

Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants