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

Mildwonkey/ps import #24412

Merged
merged 3 commits into from
Mar 20, 2020
Merged

Mildwonkey/ps import #24412

merged 3 commits into from
Mar 20, 2020

Conversation

mildwonkey
Copy link
Contributor

There are several changes in the tests, so this might be easier to read commit-by-commit - the first two commits contain the bulk of the changes and the last commit contains test updates.

I've left several hopefully-obvious "QUESTION" comments in a couple of places.

I'm concerned about the changes in the provider transformer tests - I will see what codecov has to say about my changes, but several of those tests depended on importing resources (and modules) into empy configuration. Once I refactored import to require configuration that stopped working, and made a few tests appear redundant. 🤷‍♀

`Config` in ImportOpts was any provider configuration provided by the
user on the command line. This option has already been removed in favor
of only taking the provider from the configuration loaded in the current
context.
to get the resource provider FQN from the Config
@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Mar 19, 2020
@mildwonkey mildwonkey requested a review from a team March 19, 2020 14:42
provider["registry.terraform.io/-/aws"]
module.child.aws_instance.foo (import id "bar")
provider["registry.terraform.io/-/aws"]
module.child.module.nested.aws_instance.foo
Copy link
Member

Choose a reason for hiding this comment

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

Where is this nested instance coming from? I don't see it in the test above

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see it now, it's in the new config. Should this be showing up here, since we're not importing it?

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 got the impression that it's expected to show the entire state, not just what was imported. Does that match your expectation? (the previous version of this test was importing into an empty config).

Copy link
Member

Choose a reason for hiding this comment

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

This is the graph, and not the state however. I would do some tests to see what happens when you import 1 resource, but there are other resources in the config (both existent and non-existent). This might be fine and we can fix it later, but I'm afraid that this might error out once the import process starts.

Copy link
Contributor Author

@mildwonkey mildwonkey Mar 19, 2020

Choose a reason for hiding this comment

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

Smoke test w/a config with two random_ids, one existing in state and the other not:

$terraform import random_id.dos TdTfsRX6g4o
random_id.dos: Importing from ID "TdTfsRX6g4o"...
random_id.dos: Import prepared!
  Prepared random_id for import
random_id.dos: Refreshing state... [id=TdTfsRX6g4o]

Import successful!

The resources that were imported are shown above. These resources are now in
your Terraform state and will henceforth be managed by Terraform.

$ terraform import random_id.nonexist TdTfsRX6g4o
Error: resource address "random_id.nonexist" does not exist in the configuration.

Before importing this resource, please create its configuration in the root module. For example:

resource "random_id" "nonexist" {
  # (resource arguments)
}

@mildwonkey
Copy link
Contributor Author

mildwonkey commented Mar 19, 2020

@jbardin I may need to pick your brain on the failing tests in builtin/providers/test: they are failing as (now) expected since there's no configuration. If that's an easy thing for me to figure out, I'll add it in. I'm more concerned that this will have implications for helper/schema and our providers' tests.

[UPDATE]: we spoke about this offline; the tests were invalid as written (as many of the tests modified in this PR, they depended on importing without config, which is not valid) so we removed them.

@codecov
Copy link

codecov bot commented Mar 19, 2020

Codecov Report

Merging #24412 into master will decrease coverage by 0.05%.
The diff coverage is 66.66%.

Impacted Files Coverage Δ
builtin/providers/test/provider.go 100.00% <ø> (ø)
helper/resource/testing_import_state.go 0.00% <ø> (ø)
terraform/transform_import_state.go 92.12% <62.50%> (-4.46%) ⬇️
terraform/context_import.go 100.00% <100.00%> (ø)
terraform/graph_builder_import.go 93.10% <100.00%> (ø)
dag/marshal.go 52.27% <0.00%> (-1.14%) ⬇️
command/import.go 52.19% <0.00%> (-0.98%) ⬇️
backend/remote/backend_common.go 55.63% <0.00%> (+0.70%) ⬆️
registry/client.go 69.62% <0.00%> (+1.70%) ⬆️

@mildwonkey mildwonkey requested a review from jbardin March 19, 2020 19:23
@mildwonkey mildwonkey merged commit c8d6484 into master Mar 20, 2020
@mildwonkey mildwonkey deleted the mildwonkey/ps-import branch March 20, 2020 12:15
@ghost
Copy link

ghost commented Apr 20, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants