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

Modify resolution of global variables to avoid errors on plain strings #208

Merged
merged 2 commits into from
Apr 13, 2022
Merged

Modify resolution of global variables to avoid errors on plain strings #208

merged 2 commits into from
Apr 13, 2022

Conversation

tpdownes
Copy link
Member

  • do not error when encountering string values that are not literal global variables
  • this ensures all values except literal global variables are left unmodified
  • modify unit test to test a maps of size greater than 1 and mixed types plain strings and global variables); issue went uncaught because iteration over maps does not have a guaranteed order and the code continued to succeed in limited testing

Submission Checklist

  • Have you installed and run this change against pre-commit? pre-commit install
  • Are all tests passing? make tests
  • If applicable, have you written additional unit tests to cover this
    change?
  • Is unit test coverage still above 80%?
  • Have you updated any application documentation such as READMEs and user
    guides?
  • Have you followed the guidelines in our Contributing document?

@tpdownes tpdownes requested a review from heyealex April 13, 2022 18:34
heyealex
heyealex suggested changes Apr 13, 2022
View reviewed changes
Copy link
Contributor

@heyealex heyealex left a comment

Choose a reason for hiding this comment

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

Just a couple comments that need to be updated

pkg/config/config_test.go Outdated
var testkey = "testkey"
var testkey1 = "testkey1"
var testkey2 = "testkey2"
var testkey3 = "testkey3"
bc := getBlueprintConfigForTest()
ctyMap := make(map[string]cty.Value)
err = bc.Config.ResolveGlobalVariables(ctyMap)
c.Assert(err, IsNil)

// confirm that a plain string (non-variable) is unchanged and errors
Copy link
Contributor

Choose a reason for hiding this comment

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

and errors -> and doesn't return an error
Or just remove it, whichever you prefer

Copy link
Member Author

Choose a reason for hiding this comment

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

I have updated this comment to match the test.

pkg/config/config_test.go Outdated
c.Assert(err, NotNil)
c.Assert(ctyMap[testkey], Equals, testCtyString)
c.Assert(err, IsNil)
c.Assert(ctyMap[testkey1], Equals, testCtyString)

// confirm that a literal, but not global, variable is unchanged and errors
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

I have updated this comment to match the test.

@tpdownes
Copy link
Member Author

@heyealex I think you meant to assign this back to me. I fixed the comments you asked to be changed and also updated a comment on the function itself to match the new behavior.

heyealex
heyealex reviewed Apr 13, 2022
View reviewed changes
pkg/config/config_test.go Outdated
bc := getBlueprintConfigForTest()
ctyMap := make(map[string]cty.Value)
err = bc.Config.ResolveGlobalVariables(ctyMap)
c.Assert(err, IsNil)

// confirm that a plain string (non-variable) is unchanged and errors
// confirm plain string is unchanged and does no/ error
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: no/ -> not

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeesh. Fixed.

@heyealex heyealex assigned tpdownes and unassigned heyealex Apr 13, 2022
@tpdownes
Copy link
Member Author

I would like to squash the last commit on top of the related commit on this branch. So you can review by informally saying LGTM and then I'll do that and you can approve the final branch.

Force pushing now can confuse the review process.

@tpdownes
Alter behavior of ResolveGlobalVariables
ecb4c7d 
* do not error when encountering string values that are not literal
  global variables
* this ensures all values _except_ literal global variables are left
  unmodified
* modify unit test to test a maps of size greater than 1 and mixed types
  (plain strings and global variables); issue went uncaught because
  iteration over maps does not have a guaranteed order and the code
  continued to succeed in limited testing
@tpdownes
Update test script to support validation of Packer template syntax
fe0cc93
@tpdownes tpdownes assigned heyealex and unassigned tpdownes Apr 13, 2022
@tpdownes tpdownes merged commit 44a78be into GoogleCloudPlatform:develop Apr 13, 2022
@tpdownes tpdownes deleted the fix_global_resolution branch April 14, 2022 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heyealex heyealex heyealex approved these changes

Assignees

@heyealex heyealex

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tpdownes @heyealex