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

feat: atlantis import #2783

Merged
merged 37 commits into from
Dec 23, 2022
Merged

Conversation

krrrr38
Copy link
Contributor

@krrrr38 krrrr38 commented Dec 13, 2022

what

  • introduce import workflow step
    • import step process terraform import and discard plan file, cause existing plan file cannot be used anymore.
  • import workflow run init and import steps
  • command runner can handle atlantis import ADDRESS ID
    • currently import run on a single project only. this is because we don't run import on multiple projects in the general use-case.
  • add import_requirements configuration not to edit tfstate without any reviews
  • introduce atlantis import e2e tests

why

Currently, we need to do manual terraform operations to import unmanaged resources. If support atlantis import, we can handle these operations on vcs operation.

references

discussion

  • PR is too large
  • Support remaining atlantis terraform commands #2776
    • It's possible to close this PR, if the above feature will be introduced.
  • pflag command handling
    • atlantis import -- ADDR ID
      • PROS
        • easy implementation
        • After -- comments can handled as terraform args.
      • CONS
        • pflag cannot handle it directory
    • atlantis import -a ADDR -i ID
      • PROS:
        • intuitive
        • pflag can show help
      • CONS:
        • CommentCommand need to handle command-specific parameters.
    • atlantis import ADDR ID <- choosed
      • PROS: more intuitive
      • CONS:
        • we need to manage extraArgs and newArgs in flags.
        • pflag cannot show ADDR ID usage in help like this?
        • append args into extraArgs

@krrrr38 krrrr38 force-pushed the feat-atlantis-import branch from 8a6ad94 to 65c4963 Compare December 13, 2022 00:34
@krrrr38 krrrr38 marked this pull request as ready for review December 13, 2022 00:37
@krrrr38 krrrr38 requested a review from a team as a code owner December 13, 2022 00:37
runatlantis.io/docs/using-atlantis.md Outdated Show resolved Hide resolved
server/core/config/valid/global_cfg.go Show resolved Hide resolved
server/core/runtime/import_step_runner.go Show resolved Hide resolved
server/events/comment_parser.go Show resolved Hide resolved
server/events/import_command_runner.go Show resolved Hide resolved
server/events/project_command_runner.go Show resolved Hide resolved
@krrrr38 krrrr38 marked this pull request as draft December 13, 2022 01:09
@krrrr38 krrrr38 force-pushed the feat-atlantis-import branch from 65c4963 to af56f82 Compare December 13, 2022 02:20
@krrrr38 krrrr38 marked this pull request as ready for review December 13, 2022 02:20
@krrrr38 krrrr38 force-pushed the feat-atlantis-import branch 2 times, most recently from d53c1a4 to c3f51e1 Compare December 13, 2022 04:48
@nitrocode
Copy link
Member

Wow. This looks very promising @krrrr38 !

I don't see how this can close the terraform pass through command #2776 . As amazing of a contribution as this is, it implements the import subcommand but not the state or validate or other terraform subcommands that #2776 refers to. Please don't see that as criticism. Keep going!

This is a very large PR so it will take time to review. Thank you for your efforts.

@krrrr38 krrrr38 force-pushed the feat-atlantis-import branch from c3f51e1 to ec0bdf6 Compare December 13, 2022 21:35
@nitrocode nitrocode added the waiting-on-review Waiting for a review from a maintainer label Dec 17, 2022
@nitrocode
Copy link
Member

Please give an initial review when time permits @runatlantis/maintainers

cc: @albertollamaso @Fabianoshz @Fauzyy @SSKLCP (for visibility from our more recent golang expert contributors)

Copy link
Contributor

@Fabianoshz Fabianoshz left a comment

Choose a reason for hiding this comment

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

Aside from the metrics, only minor/nitpicks from my part.

Great PR, really nice work!

server/server.go Outdated Show resolved Hide resolved
server/events/comment_parser.go Show resolved Hide resolved
runatlantis.io/docs/using-atlantis.md Outdated Show resolved Hide resolved
server/server.go Show resolved Hide resolved
@albertollamaso
Copy link
Contributor

LGTM

jamengual
jamengual previously approved these changes Dec 20, 2022
@jamengual
Copy link
Contributor

@krrrr38 Amazing work, thanks so much for this PR.
As long as you address the comments it looks good to me

@nitrocode
Copy link
Member

Looks good to me too. Please fix the conflicts and we can get this merged.

@krrrr38 krrrr38 force-pushed the feat-atlantis-import branch from ec0bdf6 to 39158d4 Compare December 21, 2022 05:50
@krrrr38 krrrr38 requested a review from Fabianoshz December 21, 2022 05:57
@@ -1,9 +1,9 @@
# Apply Requirements
# Command Requirements
Copy link
Member

Choose a reason for hiding this comment

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

Hm the renaming of this file will remove any existing links that point to this page.

There's a way to alias pages. If you keep this file rename in place, could you add a link for the original page apply-requirements.md to redirect to the new page?

Copy link
Contributor Author

@krrrr38 krrrr38 Dec 21, 2022

Choose a reason for hiding this comment

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

I added an empty page. I will try redirect plugin later.

https://vuepress-community.netlify.app/en/plugins/redirect/

Copy link
Member

Choose a reason for hiding this comment

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

cc: @chenrui333 @lilincmu in case this is what you folks mentioned a couple months ago

```
### Explanation
Runs `terraform import` that matches the directory/project/workspace.
This command discards terraform plan result. Before apply, required `atlantis plan` again.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This command discards terraform plan result. Before apply, required `atlantis plan` again.
This command discards the terraform plan result. After an import and before an apply, another `atlantis plan` must be run again.

Copy link
Member

Choose a reason for hiding this comment

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

Because of the discarding of the plan after an import, should there be another flag to auto plan a project/dir/workspace after a successful import?

Also if an import command fails can we retain the plan instead of discarding 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.

Because of the discarding of the plan after an import, should there be another flag to auto plan a project/dir/workspace after a successful import?

Currently, there is not such flag.
I thought that if users want to import one resource, autoplan is useful. But if try to import multiple imports, autoplan make the operation slow.
terraform import do not plan again, so I implements no-autoplan by default. We can provide such flag in the future as optional.

Also if an import command fails can we retain the plan instead of discarding it?

import_step_runner discard planfile if import has no err 👌

	out, err := p.TerraformExecutor.RunCommandWithVersion(ctx, filepath.Clean(path), importCmd, envs, tfVersion, ctx.Workspace)

	// If the import was successful and a plan file exists, delete the plan.
	planPath := filepath.Join(path, GetPlanFilename(ctx.Workspace, ctx.ProjectName))
	if err == nil {

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we can have an optional subcommand like we do for atlantis apply e.g. --auto-merge-disabled

For this case, perhaps a flag for --autoplan, --autoplan-on-success ? This could be added in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--autoplan looks good for import option. We can do atlantis pllan manually, so it is better to implement this after raised issue got votes.

@@ -124,6 +124,36 @@ Because Atlantis under the hood is running `terraform apply plan.tfplan`, any Te
They're ignored because they can't be specified for an already generated planfile.
If you would like to specify these flags, do it while running `atlantis plan`.

---
## atlantis import
Copy link
Member

Choose a reason for hiding this comment

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

Could you also update the tfvars section on the custom workflows page to show how to set it up for the import step? I've run into issues before where i need to pass in a custom tfvars file in order to get the import command to run correctly

https://www.runatlantis.io/docs/custom-workflows.html#tfvars-files

Copy link
Member

Choose a reason for hiding this comment

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

A test with tfvars may be useful too. Ignore me if that is overkill.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same with #2783 (comment)

@krrrr38 krrrr38 requested review from jamengual and nitrocode and removed request for Fabianoshz, jamengual and nitrocode December 22, 2022 08:57
@nitrocode nitrocode merged commit 4e0d4ec into runatlantis:main Dec 23, 2022
@nitrocode
Copy link
Member

Thank you @krrrr38 ! This change is going to help a lot!!

@jamengual
Copy link
Contributor

What an example of teamwork!!!! thanks @krrrr38 @nitrocode @Fabianoshz @albertollamaso

@krrrr38 krrrr38 deleted the feat-atlantis-import branch December 23, 2022 01:37
@nitrocode nitrocode added this to the 0.22.0 milestone Dec 23, 2022
@Dilergore
Copy link

Dilergore commented Feb 2, 2023

Hi @krrrr38,

I recently started to enable this feature in our environment. For most of the workflow steps, we have customizations (plan, apply). I've been trying to figure out how I could run terraform import from a script instead of the pre-built step.

We might have a missing feature here for custom workflows, that there is no environment var holding the ADDRESS and the ID to be able to customize this. Am I right? If so, I will raise a feature request.

@krrrr38
Copy link
Contributor Author

krrrr38 commented Feb 2, 2023

@Dilergore Thank you for using new feature.

ADDRESS ID are contains in COMMENT_ARGS in run step. But COMMENT_ARGS have 2 problems.

  • Values are contained with comma.
  • All characters are escaped by \.

This is also discussed in the Slack and there is a dirty workaround.. (slack invitation is existed in the repository README)
If you want to use more good solution, I'd be happy if you open a feature request or suggests good any ideas. Thanks.

@Dilergore
Copy link

@Dilergore Thank you for using new feature.

ADDRESS ID are contains in COMMENT_ARGS in run step. But COMMENT_ARGS have 2 problems.

  • Values are contained with comma.
  • All characters are escaped by \.

This is also discussed in the Slack and there is a dirty workaround.. (slack invitation is existed in the repository README) If you want to use more good solution, I'd be happy if you open a feature request or suggests good any ideas. Thanks.

Hi,

Thanks for the quick response, much appreciated.

I can confirm that the above is working, I was able to implement it as a custom step in the past 20 mins.

That said, I still think this should have its own dedicated variable. I will open a feature request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-review Waiting for a review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support atlantis import subcommand
6 participants