Allow to use a whole local directory as IaC source #292

eliecharra · 2021-02-24T11:02:48Z

Q	A
🐛 Bug fix?	no
🚀 New feature?	yes
⚠ Deprecations?	no
❌ BC Break	no
🔗 Related issues	#253
❓ Documentation	yes

Description

Allow to use a whole local directory as IaC source.
⚠️ Does not follow symlinks within root folder

codecov · 2021-02-26T09:19:42Z

Codecov Report

Merging #292 (c90da70) into main (ab41545) will increase coverage by 0.02%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main     #292      +/-   ##
==========================================
+ Coverage   68.87%   68.90%   +0.02%     
==========================================
  Files         258      259       +1     
  Lines        5523     5554      +31     
==========================================
+ Hits         3804     3827      +23     
- Misses       1392     1398       +6     
- Partials      327      329       +2

Impacted Files	Coverage Δ
pkg/iac/terraform/state/enumerator/file.go	`85.71% <85.71%> (ø)`
...iac/terraform/state/enumerator/state_enumerator.go	`55.55% <100.00%> (-44.45%)`	⬇️

lotoussa

Seems cool 👍

wbeuil

Need to change all multiples to multiple.

I tested it like you did which works perfectly. But we're gonna have problems with people who would like to use it with a folder like this one below which seems pretty normal in terragrunt environment if I remember well:

├── route53
│   └── terraform.tf
│   └── terraform.tfstate
└── s3
    └── terraform.tf
    └── terraform.tfstate

driftctl scan will output:

Maybe in another PR we should check that files are indeed terraform.states files before adding them to the slice.

wbeuil · 2021-02-26T10:19:04Z

pkg/iac/terraform/state/enumerator/file.go

+
+// File enumeration does not follow symlinks.
+// We may use something like this https://pkg.go.dev/github.com/facebookgo/symwalk
+// to follow links, but it allow infinite loop so be careful !


it allows ...

Sounds like I need to take English lesson on the plural rule with you 😅

wbeuil · 2021-02-26T10:30:36Z

pkg/iac/terraform/state/terraform_state_reader_acc_test.go

@@ -9,6 +9,33 @@ import (
 	"github.com/cloudskiff/driftctl/test/acceptance/awsutils"
 )

+func TestAcc_StateReader_WithMultiplesStatesInDirectory(t *testing.T) {


Multiple no S ^^

wbeuil · 2021-02-26T10:30:56Z

pkg/iac/terraform/state/terraform_state_reader_acc_test.go

+	acceptance.Run(t, acceptance.AccTestCase{
+		Paths: []string{
+			"./testdata/acc/multiples_states_local/s3",
+			"./testdata/acc/multiples_states_local/route53",


All your multiples_.... must be changed

Can you also rename the folder multiples_states_local to multiple_states_local

eliecharra · 2021-02-26T11:27:38Z

Need to change all multiples to multiple.

Sure nice catch 🙏🏻

I tested it like you did which works perfectly. But we're gonna have problems with people who would like to use it with a folder like this one below which seems pretty normal in terragrunt environment if I remember well:

This is a real question, it can totally be done in this PR but I don't know exactly what we want to do.
The .tfstate extension is not enforced by terraform at all, so I don't think we can hardcode a check on this extension.
Maybe it's the responsibility to the user to specify the pattern in the --from argument with a glob pattern for example.

I had in mind something like THAT

wbeuil · 2021-02-26T11:37:16Z

This is a real question, it can totally be done in this PR but I don't know exactly what we want to do.
The .tfstate extension is not enforced by terraform at all, so I don't think we can hardcode a check on this extension.
Maybe it's the responsibility to the user to specify the pattern in the --from argument with a glob pattern for example.

Agree we must not hardcode a check on any extension. Either it's gonna be the user who would specify a pattern like you did or if it's possible we could check if a file is a terraform state but I can't find a method in their repo that does that yet ...

I'd say we could first ship this PR with the changes I requested, then ask @sjourdan about his view and finally decide which way to go.

lotoussa · 2021-02-26T13:13:09Z

This is a real question, it can totally be done in this PR but I don't know exactly what we want to do.
The .tfstate extension is not enforced by terraform at all, so I don't think we can hardcode a check on this extension.
Maybe it's the responsibility to the user to specify the pattern in the --from argument with a glob pattern for example.

Agree we must not hardcode a check on any extension. Either it's gonna be the user who would specify a pattern like you did or if it's possible we could check if a file is a terraform state but I can't find a method in their repo that does that yet ...

I'd say we could first ship this PR with the changes I requested, then ask @sjourdan about his view and finally decide which way to go.

Can't we use magic number to check ? I'm not sure there is an identification for tf state tho.

moadibfr · 2021-02-26T13:46:56Z

The scope of ticket in this release was just to be able to use a batch of tfstate inside a repository. We talked about blob filtering but this was more intended as a first step from what I remember.

wbeuil

LGTM now

eliecharra added the kind/enhancement New feature or improvement label Feb 24, 2021

eliecharra requested a review from a team February 24, 2021 11:02

eliecharra force-pushed the allow_local_state_enumeration branch from 62b0a91 to 3bd84ab Compare February 26, 2021 09:15

eliecharra marked this pull request as ready for review February 26, 2021 09:15

lotoussa reviewed Feb 26, 2021

View reviewed changes

moadibfr previously approved these changes Feb 26, 2021

View reviewed changes

wbeuil suggested changes Feb 26, 2021

View reviewed changes

eliecharra dismissed moadibfr’s stale review via f74fe3f February 26, 2021 11:41

eliecharra force-pushed the allow_local_state_enumeration branch from 3bd84ab to f74fe3f Compare February 26, 2021 11:41

Allow to use a whole local directory as IaC source

c90da70

eliecharra force-pushed the allow_local_state_enumeration branch from f74fe3f to c90da70 Compare February 26, 2021 11:42

wbeuil approved these changes Feb 26, 2021

View reviewed changes

eliecharra merged commit d9b4ca5 into main Feb 26, 2021

eliecharra deleted the allow_local_state_enumeration branch February 26, 2021 15:16

eliecharra linked an issue Feb 26, 2021 that may be closed by this pull request

Be able to use all states within a directory #253

Closed

eliecharra mentioned this pull request Mar 5, 2021

Be able to use a glob pattern for state in --from argument #316

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to use a whole local directory as IaC source #292

Allow to use a whole local directory as IaC source #292

eliecharra commented Feb 24, 2021

codecov bot commented Feb 26, 2021 •

edited

Loading

lotoussa left a comment

wbeuil left a comment

wbeuil Feb 26, 2021

eliecharra Feb 26, 2021

wbeuil Feb 26, 2021

wbeuil Feb 26, 2021

wbeuil Feb 26, 2021

eliecharra commented Feb 26, 2021

wbeuil commented Feb 26, 2021

lotoussa commented Feb 26, 2021

moadibfr commented Feb 26, 2021

wbeuil left a comment

Allow to use a whole local directory as IaC source #292

Allow to use a whole local directory as IaC source #292

Conversation

eliecharra commented Feb 24, 2021

Description

codecov bot commented Feb 26, 2021 • edited Loading

Codecov Report

lotoussa left a comment

Choose a reason for hiding this comment

wbeuil left a comment

Choose a reason for hiding this comment

wbeuil Feb 26, 2021

Choose a reason for hiding this comment

eliecharra Feb 26, 2021

Choose a reason for hiding this comment

wbeuil Feb 26, 2021

Choose a reason for hiding this comment

wbeuil Feb 26, 2021

Choose a reason for hiding this comment

wbeuil Feb 26, 2021

Choose a reason for hiding this comment

eliecharra commented Feb 26, 2021

wbeuil commented Feb 26, 2021

lotoussa commented Feb 26, 2021

moadibfr commented Feb 26, 2021

wbeuil left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 26, 2021 •

edited

Loading