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

New option: --append to allow importing to existing workspace #151

Merged
merged 5 commits into from Jun 21, 2022

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Jun 15, 2022

For local backend, aztfy will by default ensure the output directory is empty at the very begining. This is to avoid any conflicts happen for existing user files, including the terraform configuration, provider configuration, the state file, etc. As a result, aztfy generates a pretty new workspace for users.

One limitation of doing so is users can't import resources to existing state file via aztfy. To support this scenario, this PR adds a new option --append. This option will make aztfy skip the empty guarantee for the output directory. If the output directory is empty, then it has no effect. Otherwise, it will ensure the provider setting (create a file for it if not exists). Then it proceeds the following steps.

This means if the output directory has an active Terraform workspace, i.e. there exists a state file, any resource imported by the aztfy will be imported into that state file. Especially, the file generated by aztfy in this case will be named differently than normal, where each file will has .aztfy suffix before the extension (e.g. main.aztfy.tf), to avoid potential file name conflicts.

Test

❯ AZTFY_E2E=1 go test -timeout=30m -v -run=TestMutateOutputDir ./internal/test
=== RUN   TestMutateOutputDir
Initializing...
List resources...
Import resources...
Importing /subscriptions/67a9759d-d099-4aa8-8675-e6cfd669c3f4/resourceGroups/aztfy-rg-budmjjug1 as azurerm_resource_group.round1_0
Generating Terraform configurations...
Initializing...
List resources...
Import resources...
Importing /subscriptions/67a9759d-d099-4aa8-8675-e6cfd669c3f4/resourceGroups/aztfy-rg-budmjjug2 as azurerm_resource_group.round2_0
Generating Terraform configurations...
--- PASS: TestMutateOutputDir (133.90s)
PASS
ok      github.com/Azure/aztfy/internal/test    134.036s

@magodo
Copy link
Collaborator Author

magodo commented Jun 15, 2022

@stemaMSFT and @grayzu Despite the implementaions, I'm not so sure whether the option name --mutate-output-dir is appropriate here, as it sounds too verbose. Do you have any good naming for this?

@stemaMSFT
Copy link
Member

I don't have a great alternative name for this, but I do agree that the name seems a little verbose. @grayzu let me know if you have any thoughts.

@magodo
Copy link
Collaborator Author

magodo commented Jun 20, 2022

@stemaMSFT Shall we just call this --append (meaning the output directory will be kept as is, but just "appending" new stuff), to keep the same style as the --overwrite option (meaning the output directory will be removed and recreated)?

@magodo magodo changed the title New option: --mutate-output-directory New option: --append Jun 21, 2022
@magodo magodo changed the title New option: --append New option: --append to allow importing to existing workspace Jun 21, 2022
Copy link
Contributor

@ms-henglu ms-henglu left a comment

Choose a reason for hiding this comment

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

LGTM

internal/meta/meta_impl.go Outdated Show resolved Hide resolved
internal/meta/meta_impl.go Show resolved Hide resolved
internal/meta/meta_impl.go Show resolved Hide resolved
@magodo magodo merged commit 37a492a into Azure:main Jun 21, 2022
@stemaMSFT
Copy link
Member

stemaMSFT commented Jun 23, 2022

Hey @magodo, @grayzu and I were wondering why this behavior should not be by default available for the user. Rather than having to specify --append, maybe by default appending to a directory and having to specify --overwrite instead. What are your thoughts? This may be a potential revert to modify the behavior while still addressing the overarching issues.

@magodo
Copy link
Collaborator Author

magodo commented Jun 24, 2022

@stemaMSFT The reason is "appending" to an existing directory might affecting user's data, so is regarded as a potential dangerous action. The default behavior is neither --append nor --overwrite, it is:

  • In interactive mode: check whether the directory is empty, will prompt users to clean it up if not.
  • In batch mode: check whether the directroy is empty, just error out if not.

One potential improvement is to make both modes consistent to be: check whether the directroy is empty. If not, tell the users to either use --overwrite to clean up the stuff, or use --append to append the artifacts to the dir. WDYT?

@koudaiii
Copy link
Member

The reason is "appending" to an existing directory might affecting user's data, so is regarded as a potential dangerous action.

Just Idea: If aztfy has an option such as the --dry-run or --diff, we can understand what will be changed. If we know what will be changed, we might not need empty dir.

@magodo
Copy link
Collaborator Author

magodo commented Jun 25, 2022

@koudaiii In order to understand the change that is to happen, that needs to complete the full process, including TF resource addresses specifying, importing and cfg generation. That seems not what --dry-run usually means. Whilst I still appreaciate your idea!

@koudaiii
Copy link
Member

@magodo, I see !! I misunderstood it. Thank you for your help 😄

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.

Do not require target directory to be empty Support import to existing state
4 participants