-
Notifications
You must be signed in to change notification settings - Fork 192
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
Conversation
There was a problem hiding this 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?
@@ -52,6 +52,15 @@ func newMetaImpl(rg string) (Meta, error) { | |||
return nil, fmt.Errorf("creating terraform cache dir %q: %w", tfDir, err) | |||
} | |||
|
|||
if outputDir != "" { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. This makes more sense. I'll try.
I want to clarify, do you mean outputDir
= wsp
(ref. https://github.com/magodo/aztfy/pull/15/files/8251763c0bbad7b1296acc2c448ff72c3caee413#diff-9f2fd5dbedcbf3f95aa9c36429e20b2b5ba3b63bb777338f8d0c424b8d41939bL55) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix magodo@cb216e9
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix magodo@3d9e358
Co-authored-by: magodo <wztdyl@sina.com>
@@ -52,6 +52,15 @@ func newMetaImpl(rg string) (Meta, error) { | |||
return nil, fmt.Errorf("creating terraform cache dir %q: %w", tfDir, err) | |||
} | |||
|
|||
if outputDir != "" { |
There was a problem hiding this comment.
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?
…accidentally remove anything
Co-authored-by: magodo <wztdyl@sina.com>
@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) |
There was a problem hiding this comment.
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
@magodo Thank you for your great commit. I learned from this commit. This makes more sense. |
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.
Using
-o
$ aztfy -o test_aztfy DistributedLoadTesting
No option
$ aztfy DistributedLoadTesting