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

[aztfy enhancement proposal] -o option for specified output dir #15

Merged
merged 9 commits into from
Oct 15, 2021

Conversation

koudaiii
Copy link
Member

@koudaiii koudaiii commented Oct 13, 2021

Why

aztfy currently does not allow you to specify the output dir.(only the user cached dir)
Therefore, the output dir is different for Windows / Linux / Mac. And, I need to check the cache directory every time after running aztfy.

What

I want to be able to specify the output-dir optionally.

e.g., When managing our team's terraform, I want to fix the output dir to perform the same operation on various OS in this team.

  • enhancement proposal
$ aztfy 
aztfy [option] <resource group name>

  -o string
    	Specify output dir. Default is user cache dir.
  -v	Print version

Using -o

$ aztfy -o test_aztfy DistributedLoadTesting

aztfy_—bin_aztfy-o_test_aztfy_DistributedLoadTesting—aztfy-o_test_aztfy_DistributedLoadTesting—_231×58

No option

$ aztfy DistributedLoadTesting

aztfy_—__bin_aztfy_DistributedLoadTesting—aztfy_DistributedLoadTesting—_265×62

Copy link
Collaborator

@magodo magodo left a comment

Choose a reason for hiding this comment

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

@koudaiii This PR looks really good, thank you 👍

One minor suggestion is that shall we change the semantic of outputDir here a bit to make it the directory containing all the generated stuff, rather than a directory containing sub-directories where each of them corresponds to a resource group?

internal/config/config.go Outdated Show resolved Hide resolved
@@ -52,6 +52,15 @@ func newMetaImpl(rg string) (Meta, error) {
return nil, fmt.Errorf("creating terraform cache dir %q: %w", tfDir, err)
}

if outputDir != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we change the semantic of outputDir here a bit to make it the directory containing all the generated stuff, rather than a directory containing sub-directories where each of them corresponds to a resource group? If so, we need further ensure the outputDir is an empty directory before removing everything in there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@koudaiii Yep! Shall we further check wheter the wsp here is empty? If not, we shall error out, so that we don't accidentally remove anything for the user?

Copy link
Member Author

@koudaiii koudaiii Oct 14, 2021

Choose a reason for hiding this comment

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

Thank you for the review. Ok, We check that wsp is empty. If not, we shall error out because we don't accidentally remove anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

@koudaiii koudaiii requested a review from magodo October 14, 2021 05:08
main.go Outdated Show resolved Hide resolved
@@ -52,6 +52,15 @@ func newMetaImpl(rg string) (Meta, error) {
return nil, fmt.Errorf("creating terraform cache dir %q: %w", tfDir, err)
}

if outputDir != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@koudaiii Yep! Shall we further check wheter the wsp here is empty? If not, we shall error out, so that we don't accidentally remove anything for the user?

koudaiii and others added 2 commits October 14, 2021 17:17
Co-authored-by: magodo <wztdyl@sina.com>
@koudaiii koudaiii requested a review from magodo October 14, 2021 09:05
@magodo
Copy link
Collaborator

magodo commented Oct 14, 2021

@koudaiii I've just pushed a new commit on your branch, please check whether it make sense?

wsp = outputDir

// Ensure wsp is an empty directory
stat, err := os.Stat(wsp)
Copy link
Member Author

Choose a reason for hiding this comment

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

Using os.Stat is very nice. This makes more sense. https://pkg.go.dev/os#File.Stat

@koudaiii
Copy link
Member Author

koudaiii commented Oct 14, 2021

@magodo Thank you for your great commit. I learned from this commit. This makes more sense.

@magodo magodo merged commit be20bc4 into Azure:main Oct 15, 2021
@koudaiii koudaiii deleted the koudiaii/specify-output-dir branch October 15, 2021 03:26
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.

None yet

2 participants