Adding Chdir option like terraform #12264

teddylear · 2023-02-12T02:19:02Z

feat: Adding chdir option same as terraform so packer can run files in another directory

Some open questions:

What docs and help message do I have to update for this feature?
Is there any other testing required? I tried to copy existing tests for main that were present.

Please let me know if there's anything I should change/add and thank you for the review in advance!

Closes #12183

…s in another directory

h-dav · 2023-03-03T13:55:49Z

main.go

+		}
+		if strings.HasPrefix(arg, argPrefix) {
+			argPos = i
+			argValue = arg[len(argPrefix):]


Suggested change

argValue = arg[len(argPrefix):]

argValue = arg[len(argPrefix):]

break

In the event there is other args after -chdir

Thanks for the review! Added this change to PR. Please let me know if there's anything else I should change.

Bit late to the party, but as a counterpoint, I would suggest maybe going through all the arguments maybe, so that if someone defines --chdir twice, we can error.
That, or we could arbitrarily decide to use the last one as truth value, but this may end-up confusing imho, so I would think erroring might be better.

That being said, I see that for the other global argument we support (-machine-readable), we don't do this, so I presume the subcommand will take care of erroring in this case.
This would make for a good argument in favour of not checking for this use-case here, or maybe adopting a similar duplication-check for both.

teddylear · 2023-05-06T17:54:38Z

@h-dav @nywilken Just pinging for visibility, is there any other testing and documentation I should do for this feature? Not sure how to else to test this feature. Or if this is something that you don't want added that's completely understandable as well. Thanks!

lbajolet-hashicorp

Hi @teddylear,

Thanks for submitting this PR, and apologies for the long wait before a review.

The idea is interesting, and I can imagine this being an option that is useful. I'm less convinced about the implementation.
The code is not trivial, and forces one convention for passing the argument, which is somewhat inconsistent with what exists in Packer now.
I understand the point of having something equal to Terraform, which from a global company/product view is good, but I would think this may feel unnecessarily constricted compared to what else we support.

Given how prevalent the usage of environment variables is in Packer, and since the CLI library we use does not support (AFAICT) the concept of global arguments, I tentatively think this kind of change may fare better as a new environment variable?
This is less obvious than a flag for sure, but this would be more in line with how Packer behaves otherwise, and would make for a much simpler implementation as we wouldn't need to parse the value from the raw command-line arguments.

What do you think?

cc @nywilken, what's your take on that also?

lbajolet-hashicorp · 2023-09-25T18:43:16Z

main.go

+		}
+		if strings.HasPrefix(arg, argPrefix) {
+			argPos = i
+			argValue = arg[len(argPrefix):]


Bit late to the party, but as a counterpoint, I would suggest maybe going through all the arguments maybe, so that if someone defines --chdir twice, we can error.
That, or we could arbitrarily decide to use the last one as truth value, but this may end-up confusing imho, so I would think erroring might be better.

lbajolet-hashicorp · 2023-09-25T18:46:34Z

main.go

+		}
+		if strings.HasPrefix(arg, argPrefix) {
+			argPos = i
+			argValue = arg[len(argPrefix):]


That being said, I see that for the other global argument we support (-machine-readable), we don't do this, so I presume the subcommand will take care of erroring in this case.
This would make for a good argument in favour of not checking for this use-case here, or maybe adopting a similar duplication-check for both.

lbajolet-hashicorp · 2023-09-25T18:48:26Z

main.go

+	var argPos int
+
+	for i, arg := range args {
+		if !strings.HasPrefix(arg, "-") {


With respect to the other global arguments we support, AFAICT, they can be specified anywhere in the command-line.
While Terraform seems to enforce the location of the argument, I would think that in our case, in order to remain consistent with the rest of the options, we should allow it to be specified anywhere.

lbajolet-hashicorp · 2023-09-25T18:53:24Z

main.go

+			break
+		}
+		if arg == argName || arg == argPrefix {
+			return "", args, fmt.Errorf("must include an equals sign followed by a directory path, like -chdir=example")


Again with respect to the other arguments we support, they can be specified like what is printed out here or with variations.
Typically, we can specify options with either one or two -, with the exception of the -machine-readable option, which is hard-coded with one - prefix.
The = for the value is also not strictly required, options can be specified in two parts, with the value being the next argument, if unspecified as part of the introduction.
For consistency with the other arguments, I would prefer this keeps the same convention, but this does complexify the code.

teddylear · 2023-09-27T03:45:34Z

@lbajolet-hashicorp Thanks for the review! I agree if most things configurations are easier to do via environment variable, I can add this new configuration that way. Let me update PR this week to do that instead unless there is any argument against that approach.

nywilken · 2023-10-27T15:57:02Z

@teddylear thanks for pushing this forward. @lbajolet-hashicorp I see your point but TBH I'm having a hard time working through the CLI invocations with flags or a environment variable. Looking at the referenced issue and the use of Terraform I think there might be some gaps in the approach. Especially when it comes to templates that using path.root and path.cwd ( I don't know how they would behave in Packer when using the proposed implementation). @teddylear these are things that would need to documented.

That said, just looking at the possible invocations which would make more sense to a user?

~>  packer --chdir=dev validate --var-file=variables.pkvars.hcl template.pkr.hcl
~> packer --chdir=dev build --var-file=variables.pkvars.hcl template.pkr.hcl
// Or something like 
~> packer validate --chdir=dev --var-file=variables.pkvars.hcl template.pkr.hcl
~> packer build --chdir=dev --var-file=variables.pkvars.hcl template.pkr.hcl

The benefit of having chdir be a sub command flag is that we can add it to only those commands that would benefit and reuse the MetaArgs type for handling the flag.

One confusing thing to me is if chdir is specified then are var-file values relative the the cwd directory or to the overwritten directory?

~> packer validate --chdir=dev --var-file=../global.pkrvars.hcl .

If we go with an environment variable what would that look like?

My reservation to the env variables is that on Windows setting the env might require a bit more typing then on Unix which makes the argument just change directory manually easier for "me" to consume. But I'm just sharing my thoughts as a user.

nywilken · 2023-10-30T18:27:57Z

Hi @gammore we started looking into this request future and think that we might need more information on use cases to better understand the why and how. Do you care to share more details on how such a feature could be helpful to you?

@teddylear thanks again for driving this work. Your contributions are greatly appreciated, as is all contributions. We are looking for more insight on this feature to figure out how we want to proceed. Taking TF implementation does seem right given the information we have. Feel free to share any thoughts.

gammore · 2023-10-30T18:45:09Z

Hi @gammore we started looking into this request future and think that we might need more information on use cases to better understand the why and how. Do you care to share more details on how such a feature could be helpful to you?

Of course!

What I have is the following folder structure:

project/
  Makefile
  packer/
    frontend/
      packer.pkr.hcl
    backend/
      packer.pkr.hcl
    variables/
      xxx

The idea is to have a Makefile on the root of the repo that is able to build the different projects without having to recurse to "cd" into folders, modifying the user's PWD.

For example

make frontend

Would execute:

packer init -chdir packer/frontend/packer.pkr.hcl
packer build -chdir packer/frontend/packer.pkr.hcl -var-file packer/variables/variables.pkrvars.hcl

I actually use the same pattern to apply automatically terraform projects, like make ecommerce actually executes:

terraform plan -chdir ecommerce
terraform apply -chdir ecommerce

With this same pattern, I can make wordpress or make backend for example.

I hope this information helps clarify the issue.

Thanks for taking a look at this!

lbajolet-hashicorp · 2023-10-30T18:54:15Z

Hi @gammore,

Looking from the outside, I would think make may provide a solution on its own, by adding extra makefiles though, but the idea would be:

project/
  Makefile
  packer/
    frontend/
      packer.pkr.hcl
      Makefile
    backend/
      packer.pkr.hcl
      Makefile
    variables/
      xxx

This way you can invoke $(MAKE) -C frontend as the command for the top-level target, which won't force you to cd into one of the subdirectories directly, instead deferring that responsibility to make.

Something like:

.PHONY: all frontend backend

all: frontend backend

frontend:
    $(MAKE) -C frontend

backend:
    $(MAKE) -C backend

Would that work? Or do you have something preventing this kind of workflow?

gammore · 2023-11-01T08:05:16Z

Hi @gammore,

Looking from the outside, I would think make may provide a solution on its own, by adding extra makefiles though, but the idea would be:
project/
  Makefile
  packer/
    frontend/
      packer.pkr.hcl
      Makefile
    backend/
      packer.pkr.hcl
      Makefile
    variables/
      xxx
This way you can invoke $(MAKE) -C frontend as the command for the top-level target, which won't force you to cd into one of the subdirectories directly, instead deferring that responsibility to make.

Something like:
.PHONY: all frontend backend

all: frontend backend

frontend:
    $(MAKE) -C frontend

backend:
    $(MAKE) -C backend
Would that work? Or do you have something preventing this kind of workflow?

Hi!

Yes, that's not a bad idea, I might implement that as a work around!. With this there is quite a bit of code repetition though (just a few lines, with the build).

An ideal scenario with this feature would be to have a makefile like this on the repository root:

.PHONY:  build

build:
  packer init -chdir packer/$PARAM/packer.pkr.hcl
  packer build -chdir packer/$PARAM/packer.pkr.hcl -var-file packer/variables/variables.pkrvars.hcl

I don't remember on top of my head how makefile parameters work (they require a bit more code than this), but in a perfect world, with the same code I would be able to run:

make build frontend
make build backend
make build anynewprojectadded

Adding a new project would be simply to add its folder and packer.pkr.hcl file and run make build project without changing the Makefile.

As said, that would be the ideal scenario, but your solution works for the moment. Thanks for your time!

feat: Adding chdir option similar to terraform so packer can run file…

32d999d

…s in another directory

teddylear marked this pull request as ready for review February 12, 2023 02:29

teddylear requested a review from a team as a code owner February 12, 2023 02:29

h-dav reviewed Mar 3, 2023

View reviewed changes

teddylear added 2 commits March 8, 2023 17:35

fix: add break statement for chdir option

0fe82bb

fix: format

47103ba

teddylear requested a review from h-dav March 8, 2023 22:59

nywilken added this to the 1.9.0 milestone May 1, 2023

nywilken removed this from the 1.9.0 milestone May 30, 2023

lbajolet-hashicorp requested changes Sep 25, 2023

View reviewed changes

nywilken added the enhancement label Oct 23, 2023

nywilken added the stage/needs-investigation label Oct 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Chdir option like terraform #12264

Adding Chdir option like terraform #12264

teddylear commented Feb 12, 2023 •

edited

Loading

h-dav Mar 3, 2023

h-dav Mar 3, 2023

teddylear Mar 8, 2023

lbajolet-hashicorp Sep 25, 2023

lbajolet-hashicorp Sep 25, 2023

teddylear commented May 6, 2023

lbajolet-hashicorp left a comment

lbajolet-hashicorp Sep 25, 2023

lbajolet-hashicorp Sep 25, 2023

lbajolet-hashicorp Sep 25, 2023

lbajolet-hashicorp Sep 25, 2023

teddylear commented Sep 27, 2023

nywilken commented Oct 27, 2023

nywilken commented Oct 30, 2023

gammore commented Oct 30, 2023

lbajolet-hashicorp commented Oct 30, 2023

gammore commented Nov 1, 2023

	argValue = arg[len(argPrefix):]
	argValue = arg[len(argPrefix):]
	break

Adding Chdir option like terraform #12264

Are you sure you want to change the base?

Adding Chdir option like terraform #12264

Conversation

teddylear commented Feb 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teddylear commented May 6, 2023

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teddylear commented Sep 27, 2023

nywilken commented Oct 27, 2023

nywilken commented Oct 30, 2023

gammore commented Oct 30, 2023

lbajolet-hashicorp commented Oct 30, 2023

gammore commented Nov 1, 2023

teddylear commented Feb 12, 2023 •

edited

Loading