Skip to content
This repository has been archived by the owner on Sep 13, 2023. It is now read-only.

Add a new state_mv method #13

Merged
merged 12 commits into from
Oct 15, 2021
Merged

Add a new state_mv method #13

merged 12 commits into from
Oct 15, 2021

Conversation

karajan1001
Copy link
Contributor

fix #12

  1. Add a new rename method to TerraformBackend
  2. Add a test for rename method

@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2021

Codecov Report

Merging #13 (19abda5) into master (fc779c4) will increase coverage by 0.32%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
+ Coverage   79.25%   79.58%   +0.32%     
==========================================
  Files           5        5              
  Lines         188      191       +3     
==========================================
+ Hits          149      152       +3     
  Misses         39       39              
Impacted Files Coverage Δ
tpi/terraform.py 93.67% <100.00%> (+0.24%) ⬆️

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 fc779c4...19abda5. Read the comment docs.

@karajan1001
Copy link
Contributor Author

One problems. I hadn't found a way to modify the name inside the instance's attribute , in terraform.tfstate

  "resources": [
    {
      "type": "iterative_machine",
      "name": "new_name",
      "provider": "provider[\"registry.terraform.io/iterative/iterative\"]",
      "instances": [
        {
          "attributes": {
            "name": "still_old_name",
            }
}
}

tpi/terraform.py Outdated Show resolved Hide resolved
tpi/terraform.py Outdated Show resolved Hide resolved
tpi/terraform.py Outdated Show resolved Hide resolved
tpi/terraform.py Outdated Show resolved Hide resolved
@0x2b3bfa0
Copy link
Member

[...] modify the name inside the instance's attribute , in terraform.tfstate

Terraform state files are intended to represent already existing infrastructure, not as a replacement or alternative representation for .tf or .tf.json files. Modifying an attribute would require performing the change in the latter and running terraform apply again.

Even if you did so, our Terraform provider doesn't support renaming instances. If you change its name you would also have to destroy/create it again.

This doesn't have an easy solution, because we're using the Terraform provider just to run a bunch of cloud API calls sequentially and don't have granular resources nor Read/Update methods for any of them. 🙈 🙊 🙉

Deeply related to iterative/terraform-provider-iterative#175 (comment)

@pmrowla
Copy link
Contributor

pmrowla commented Sep 23, 2021

@karajan1001 Modifying the instance attributes isn't really part of the requirement from the DVC side. Renaming the resource itself via terraform state mv is good enough since we can still access the resource (to destroy it when needed) by the new name, even if some of the instance attributes don't match up exactly. In DVC we don't actually use the name instance attribute at all, we only look things up by the resource name.

tpi/terraform.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

what about setting a timeout exclusion for just this test?

setup.cfg Outdated Show resolved Hide resolved
tests/test_terraform.py Show resolved Hide resolved
@pmrowla pmrowla changed the title Add a new rename method Add a new state_mv method Oct 1, 2021
tpi/terraform.py Outdated
Comment on lines 155 to 156
with self.make_tf(name) as tf:
tf.cmd("state mv", f"{source}", f"{destination}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about this some more, it's non-intuitive that most of the TPI functions depend on per-iterative-machine workspace directories created in make_tf (meaning that source and destination here should always be iterative-machine.<...> names), it makes more sense for TPI to be generic, but that doesn't have to be handled in this PR

see: #15

pmrowla
pmrowla previously approved these changes Oct 1, 2021
@casperdcl casperdcl added the enhancement New feature or request label Oct 1, 2021
Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

minor comment

tpi/terraform.py Outdated Show resolved Hide resolved
karajan1001 and others added 12 commits October 14, 2021 15:11
1. Add a new rename  method to TerraformBackend
2. Add a test for rename method
Co-authored-by: Casper da Costa-Luis <[email protected]>
Co-authored-by: Casper da Costa-Luis <[email protected]>
Co-authored-by: Casper da Costa-Luis <[email protected]>
Co-authored-by: Casper da Costa-Luis <[email protected]>
Timeout only act on state_mv

Co-authored-by: Casper da Costa-Luis <[email protected]>
Co-authored-by: Casper da Costa-Luis <[email protected]>
Comment on lines +134 to +140

def state_mv(self, source, destination, **kwargs):
"""Retain an existing remote object but track it as a
different resource instance address.
"""
assert source and destination
self.tf.cmd("state mv", source, destination)
Copy link
Contributor

Choose a reason for hiding this comment

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

great 😅 thanks to #17 doesn't this function now become unnecessary?

@pmrowla WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll still need the wrapper for terraform state mv in DVC

Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

lgtm apart from minor utility comment/query

@pmrowla pmrowla merged commit b80ed24 into iterative:master Oct 15, 2021
@karajan1001 karajan1001 deleted the add_rename branch October 15, 2021 03:44
@casperdcl
Copy link
Contributor

/tag v2.1.0 b80ed24

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add rename method to TerraformBackend
5 participants