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

make TerraformState classmethods more clear #20

Merged
merged 3 commits into from
Sep 4, 2020

Conversation

marcoceppi
Copy link
Contributor

@marcoceppi marcoceppi commented Sep 3, 2020

This PR came about because in a previous update I abused the load classmethod as a way to update TerraformState after the modify_state hook runs. Since it's a classmethod it spawns a new class object instead of loading over it's existing state. This introduces TerraformState.from_file and TerraformState.from_state classmethods which expand on what the load classmethod was doing previously. load now updates the existing invocation and the state parsing code has been moved to a static method.

The two reservations I have and am looking to get explicit feedback on is:

  1. Should load be renamed to update in order to better highlight it's purpse?
  2. Should from_state be renamed to something like from_str given the data passed to it is likely a string representation of state?

@marcoceppi marcoceppi requested a review from kapilt September 3, 2020 01:20
@marcoceppi marcoceppi force-pushed the update-terraform-json branch from b2969f2 to 2e8e1f7 Compare September 3, 2020 01:21
@codecov
Copy link

codecov bot commented Sep 3, 2020

Codecov Report

Merging #20 into master will increase coverage by 0.23%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
+ Coverage   92.27%   92.51%   +0.23%     
==========================================
  Files           8        8              
  Lines         440      454      +14     
==========================================
+ Hits          406      420      +14     
  Misses         34       34              
Impacted Files Coverage Δ
pytest_terraform/exceptions.py 100.00% <100.00%> (ø)
pytest_terraform/tf.py 96.19% <100.00%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ceca709...0395b0c. Read the comment docs.

@marcoceppi marcoceppi force-pushed the update-terraform-json branch from 2e8e1f7 to 6c90796 Compare September 3, 2020 01:29
@kapilt
Copy link
Contributor

kapilt commented Sep 3, 2020

  1. load to update, yes. previously it was a constructor returning new instance, now its a mutation that should be reflected as such
  2. from_string sounds good, where do we pass in an object just on the hook mutation writer? if we are passing in a object, i would expect a shallow copy rather than a shared copy. ie state_d = dict(state.dict)
  3. we should have some typing annotations and/or doc strings inline, the current notion of state (from_state) as Union[TerraformDate, Dict[Any][Any]] is a little implicit to infer else.

@marcoceppi
Copy link
Contributor Author

  1. sounds good, updated
  2. nothing actively uses the from_state / from_string except for from_file and some unit tests bits. It's more there to distinguish from_file, used occasionally in the unit tests, and in the event we need it in the future. All the hook mutation bits are here:
    test_api = self.runner.apply()
    tfstatejson = test_api.save()
    self.config.hook.pytest_terraform_modify_state(tfstate=tfstatejson)
    test_api.load(tfstatejson)
    test_api.save(module_dir.join("tf_resources.json"))
    where we save the state to a new TerraformStateJson object, use that shared object because of how pytest hooks work, then update the TerraformState with any changes from the hook execution
  3. Yeah, this is a good point. I can add type annotations and doc strings

@marcoceppi marcoceppi force-pushed the update-terraform-json branch from 92688c3 to 5e4e619 Compare September 3, 2020 16:12
@marcoceppi marcoceppi force-pushed the update-terraform-json branch from 1c78ff2 to 0395b0c Compare September 3, 2020 16:50
@marcoceppi
Copy link
Contributor Author

Update TerraformState and TerraformStateJson with annotations and docstrings

Copy link
Contributor

@kapilt kapilt left a comment

Choose a reason for hiding this comment

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

thanks, lgtm

@marcoceppi marcoceppi merged commit 0c68fa0 into master Sep 4, 2020
@marcoceppi marcoceppi deleted the update-terraform-json branch September 4, 2020 17:04
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.

2 participants