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

Add support for bulk imports #22227

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

tmccombs
Copy link
Contributor

Fixes #22219

@tmccombs
Copy link
Contributor Author

I should probably write tests for this before it is merged, but I'd like feedback on what I have first.

@pselle
Copy link
Contributor

pselle commented Jul 29, 2019

In general, this feature would be much appreciated! It's a proposal that's been spotted on the backlog before, and I imagine it will help a lot of users onboarding to Terraform, so thank you!

Agree that tests (and documentation for terraform.io) are good to add before merging this. website/docs/import/index.html.md would be a good spot for such docs, and since this adds options, something in usage: website/docs/import/usage.html.md

I hope @apparentlymart will pardon a tag on this since he has ideas surrounding this, and likely more feedback, so that this ends up on a path towards something we merge, but I'll put some of the thoughts discussed here so you might respond @tmccombs :)

Martin pointed out* that the approach of zero arguments reading from standard input can be confusing for users. In general, Terraform (the software) tries to give feedback on what it expects to do, or error if it can't, and that sort of approach (immediate feedback).

A prior proposal (not on GitHub) suggested that users define a file that would be used to define the resources to import, and this was an example:

import "aws_vpc" "main" {
  tags = {
    Name        = "main"
    Environment = "PROD"
  }
}
import "aws_subnet" "us-west-2a" {
  vpc_id            = "${aws_vpc.main.id}"
  availability_zone = "us-west-2a"
}
import "aws_subnet" "us-west-2b" {
  vpc_id            = "${aws_vpc.main.id}"
  availability_zone = "us-west-2b"
}

If your current approach was modified from reading lines of standard input to reading a file with the following information, ideally with a flag pointing to the file, that seems more in line with the Terraform approach:

Each line should have a single resource pair containing the resource address and id separated by a space.

For example:

terraform import -bulk=path/to/defining_file

Where file is something like this in the first pass:

server.example i-abcd1234
server.another i-def3456

But would be more understandable in another format, most likely, so that the file (.tfimport?) is less opaque, and a little more syntax rather than relying on spaces and line endings.

* I might be corrected on this

@apparentlymart
Copy link
Member

The idea of using HCL for this was coming from some broader ideas around import, such as being able to import by more than just a single string id (instead, arguments similar to data sources) and resolving the fact that right now there's no way to give Terraform any information during import about dependencies, and so imported objects can later fail to destroy properly due to incorrect ordering.

However, I'm open to moving forward with something that just directly addresses this one problem (performing a set of import operations all at once with a single, atomic state write); the other parts of this have bigger design questions associated and so it would be a shame to block solving this aspect of it on having to resolve the bigger problems.

With that said, this would be my suggestion for how to move forward here:

  • As Pam suggested, I think it's better to put the bulk option behind an explicit option rather than making it default behavior so that terraform import with no arguments can still produce the usage information. (Having it silently block on reading from stdin is the behavior I was worried about being confusing.)
  • I expect reading from stdin is useful when using this in conjunction with a helper program to generate the data; our historical convention for that has been (following the example in a number of Unix-ish programs) to treat the name - as a way to explicitly say "stdin", like terraform import -bulk=-. Although some operating systems provide an existing way to specify stdin, this convention allows us to document the same procedure cross-platform, at the expense of not supporting the (unlikely, I think) situation of importing from a file literally named -.
  • If we were to later on do something like the bigger rework of the import mechanism with a more complex input file, I think we could find a way to formulate it so that these two mechanisms could coexist if that feels necessary, though it would be a significant enough architecture change that we might elect to use this opportunity to make breaking changes to the terraform import command anyway, depending on tradeoffs we will only become aware of during the detailed design process (which is not something we're looking at in the short term).

@tmccombs
Copy link
Contributor Author

Thanks for the feedback. I will change it to use a seperate flag, and add some documentation and tests.

@tmccombs
Copy link
Contributor Author

tmccombs commented Aug 1, 2019

I've added documentation and a unit test.

@tmccombs
Copy link
Contributor Author

tmccombs commented Aug 7, 2019

I changed the import format to use JSON instead, because I realized there are resource ids that contain spaces, and I didn't want to re-invent a new escaping mechanism.

@tmccombs
Copy link
Contributor Author

Is there anything blocking this on my side? I realize approvers are busy, but want to make sure this isn't getting held up because I missed something.

@pselle
Copy link
Contributor

pselle commented Aug 12, 2019

Requesting @apparentlymart's thoughts :)

@james-callahan
Copy link

@tmccombs could you rebase?

at the expense of not supporting the (unlikely, I think) situation of importing from a file literally named -.

In such a case you could write -bulk=./-

@tmccombs
Copy link
Contributor Author

well, that was more difficult than I would have hoped, but it has been rebased.

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@tmccombs
Copy link
Contributor Author

I now think a better approach for this might be to support an import block similar to the moved block introduced in terraform 1.1.

I'll see if I can put together a PR.

@magodo
Copy link
Contributor

magodo commented Sep 20, 2022

@crw
Copy link
Collaborator

crw commented Sep 23, 2022

See also: #30015

@ehud-eshet
Copy link

Hi all,

Pull request #30015 is pretty similar.
The main difference:
#30015 allows specifying multiple pairs of resource and id in the command line while in this pull requests the list of pairs must be provided in a text file.
I don't care which of the two is merged.
This enhancement is long waited.
Please choose one and merge it.

Many thanks.

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

Successfully merging this pull request may close these issues.

Ability to import in bulk
9 participants