Target module callers when local path module is updated #1528

exoego · 2024-02-13T08:34:34Z

Probably adresses #245 partially.

Issue

Let's say we have directory structures below:

modules/
  foo/
    main.tf
    tfaction_module.yaml
  bar/
    main.tf
    tfaction_module.yaml
workspace/
  production/
    tfaction.yaml
    main.tf
  staging/
    tfaction.yaml
    main.tf
  development/
    tfaction.yaml
    main.tf

Workspaces prod, staging and development use local-path modules modules/foo and modules/bar.

If the module modules/foo/main.tf is updated, unfortunately, tfaction can not plan/apply on its use-sites (production, staging and development).
Users need to update workspace/production/main.tf and such slightly.

Solution

After this PR, tfaction can plan and apply on module callers (use-site) even if each caller is not changed.

This new behavior is disabled by default.
Users needs to configure it in trfaction-root.yaml:

update_local_path_module_caller:
  enabled: true

Implementation detail

hashicorp/terraform-config-inspect is used to list up all local-path modules
Based on the inspection result, list-module-callers action creates a mapping from a module name to its caller.
Based on this mapping, a module caller will also be targeted if its modules are updated.

suzuki-shunsuke

Thank you for your contribution!
I have several comments.

First, I really appreciate it if you write an issue or a discussion before sending a pull request.

Second, I'm sorry that the description of #245 isn't enough, but this issue is about not local-path modules but GitHub Source modules. As we mentioned in the blog post and the document, we're recommending GitHub Source Modules rather than local-path modules

Third, I don't think changes of local-path modules should be applied to all callers on the same pull request.

~~Sometimes you~~ Some users would like to apply changes by separated pull requests and apply changes one by one.
The code owners may be different per caller.
~~You~~ Some users may want to apply changes to the specific callers first. For example, ~~you~~ they may want to apply changes to develop environments first and apply changes to production environments later.
If the local-path module is used by a lot of callers, it may be hard to review the result of terraform plan

suzuki-shunsuke · 2024-02-13T09:36:01Z

Users need to update workspace/production/main.tf and such slightly.

Maybe you're aware, but you can also use target:<target> labels.
JFYI

exoego · 2024-02-13T09:59:08Z

Third, I don't think changes of local-path modules should be applied to all callers on the same pull request.

Sometimes you would like to apply changes by separated pull requests and apply changes one by one.

No, we want to apply changes to all callers in a single PR, in my organization.
Because, folks tend to

forget to update some of callers when they update a sub module, and
get confused when they update workspace X and the TF plan includes changes due to the sub module Y recently updated by someone else.

When we need to update callers granularly, we use GitHub module or other version-controlled options.

    source = "github.com/my-org/terraform-module-fooo?ref=v1.2.3"

I can understand other users may not need this new behavior.
I am happy to put this new behavior under a new input something like update_local_path_module_caller: boolean.

suzuki-shunsuke · 2024-02-13T10:13:00Z

forget to update some of callers when they update a sub module, and

get confused when they update workspace X and the TF plan includes changes due to the sub module Y recently updated by someone else.

I understand your motivation.

I can understand other users may not need this new behavior.

Yes, so I don't want to apply this change by default. Users would be surprised if we change the default behaviour.

I am happy to put this new behavior under a new input something like update_local_path_module_caller: boolean.

One of ideas is to add the setting like update_local_path_module_caller: boolean to tfaction-root.yaml.

exoego · 2024-02-14T02:04:19Z

@suzuki-shunsuke Added a new extensible configuration object:

update_local_path_module_caller: z.optional(
    z.object({
      enabled: z.optional(z.boolean()),
    }),
  ),

as same as update_related_pull_requests. 246a10d

suzuki-shunsuke · 2024-02-17T05:11:13Z

list-targets/action.yaml

@@ -39,15 +39,26 @@ runs:
      env:
        HEAD_SHA: ${{github.event.pull_request.head.sha}}

+    - run: go install github.com/hashicorp/terraform-config-inspect@latest


We should pin the version. latest is unstable and insecure.
And this step requires Go, so we should clarify the requirement.

We can also install this tool by aqua.

https://github.com/aquaproj/aqua-registry/tree/main/pkgs/hashicorp/terraform-config-inspect

In that case, we need to ask users to update aqua.yaml, which isn't ideal.

Pinned the version a7d4635

I did not touch aqua, since I am not familiar with aqua and you said it is not ideal.

Golang dpendency declared in 1aba475

suzuki-shunsuke · 2024-02-17T05:15:08Z

list-module-callers/src/index.ts

+        }
+
+        const tfDir = path.dirname(tfFile);
+        const inspection = JSON.parse(child_process.execSync(`/home/runner/go/bin/terraform-config-inspect --json ${tfDir}`).toString("utf-8"));


The path to terraform-config-inspect depends on the environment.

Added GOBIN/GOPATH detection.
3acf082

suzuki-shunsuke · 2024-02-17T06:09:03Z

list-targets-with-changed-files/src/run.ts

@@ -153,16 +153,22 @@ export const run = (input: Input): TargetConfig[] => {
    }
  }

+  const moduleCallerMap = JSON.parse(core.getInput("module_callers") || "{}");


Please don't call core.getInput here to make the function easy to test.
Please call core.getInput in main instead.

Ah, that is nice.
Fixed in e150731

suzuki-shunsuke · 2024-02-17T06:13:12Z

Does the current implementation support nested modules?
If the working directory A calls the module B, and the module B calls the module C, and if the module C is updated,
is the job run on the working directory A?

suzuki-shunsuke · 2024-02-17T06:17:32Z

list-targets/action.yaml

+      id: global-config
+    - uses: suzuki-shunsuke/tfaction/list-module-callers@main
+      id: list-module-callers
+      if: steps.global-config.outputs.disable_update_local_path_module_caller == 'false'


It seems the feature is enabled by default, but as I said I want to disable this feature by default.

#1528 (comment)

And I think the output name should be update_local_path_module_caller because the feature is disabled by default.

Rnamed in f6d9d8b

exoego · 2024-02-17T06:46:25Z

Does the current implementation support nested modules?
If the working directory A calls the module B, and the module B calls the module C, and if the module C is updated,
is the job run on the working directory A?

Yes, it does support nested module calls.
These functions supports nested module calls.
https://github.com/suzuki-shunsuke/tfaction/pull/1528/files#diff-303affc8621a9e2bb810064d3f0633fdf227ce745d68ea05a07ea8f0fd2415f3

suzuki-shunsuke · 2024-02-17T08:01:37Z

Does the current implementation support nested modules?
If the working directory A calls the module B, and the module B calls the module C, and if the module C is updated,
is the job run on the working directory A?

Yes, it does support nested module calls. These functions supports nested module calls. https://github.com/suzuki-shunsuke/tfaction/pull/1528/files#diff-303affc8621a9e2bb810064d3f0633fdf227ce745d68ea05a07ea8f0fd2415f3

I see. Thank you.

exoego · 2024-03-12T23:48:54Z

@suzuki-shunsuke Could you take a look again on this

suzuki-shunsuke · 2024-03-16T01:14:29Z

list-module-callers/package.json

+    "@types/node": "20.11.8",
+    "@vercel/ncc": "0.38.1",
+    "typescript": "5.3.3",
+    "vitest": "^1.2.2"


We use jest, not vitest.
I'm not familiar with vitest.
So we should unify the library to jest if there is no special reason.

Replaced with jest 8b1f1ca

Vitest is a very fast testing framework and its API/config is highly compatible with Jest.
https://vitest.dev/

I see. Thank you for your explanation.

I think it's okay to replace Jest with Vitest if Vitest is better than Jest.
But we should not do this in this pull request.
We should open another pull request.

list-module-callers/package.json

…-inspect

suzuki-shunsuke · 2024-03-16T02:29:12Z

8d83b3e Specify the install path of terraform-config-inspect

suzuki-shunsuke · 2024-03-16T02:49:44Z

Release Note

Features

#1528 Support running CI against working directories where depend on a updated local path module

By default, tfaction runs CI on only working directories where any code is updated.
This means even if a working directory depends on a module out of the working directory and the module is updated, CI isn't run on the working directory.

e.g.

A working directory A depends on local path module B
Module B is located out of the working directory A
In a pull request C, working directory A isn't changed but the module B is changed
Then CI isn't run on the working directory A by default

working directory A/
modules/
  module B

This update enables you to run CI on the working directory A too.
The feature is disabled by default, so you have to enable the feature explicitly.

tfaction-root.yaml

update_local_path_module_caller:
  enabled: true

This feature depends on terraform-config-inspect, so you have to install it.
Same with other tools, you can install terraform-config-inspect with aqua.

e.g.

packages:
  - name: hashicorp/terraform-config-inspect
    version: a34142ec2a72dd916592afd3247dd354f1cc7e5c

In that case, Go is required.

If this feature is enabled, when a module is updated in a pull request, CI is run on working directories depending on the module.
The module dependency is checked recursively.
For example, in the above case if the module B depends on a module C and module C is updated in a pull request,
CI is run on the working directory A even if the working directory A and the module B aren't updated.

…t to a temporal directory It seems that action can't refer files on other actions

You have to install terraform-config-inspect yourself

…ate-module-use-site # Conflicts: # list-module-callers/test/lib.test.ts

exoego · 2024-03-16T05:09:01Z

@suzuki-shunsuke Replaced Vitest with Jest

suzuki-shunsuke · 2024-03-16T05:09:49Z

I changed how to install terraform-config-inspect.
tfaction doesn't install it automatically anymore, so you have to install it yourself.
As described in #1528 (comment) , you can install it with aqua.

I said,

In that case, we need to ask users to update aqua.yaml, which isn't ideal.

But this feature is disabled by default so users have to configure tfaction explicitly.
This isn't a breaking change.
So I think we can accept this change.

This has both pros and cons.

pros
- We don't have to update terraform-config-inspect
- We can remove the code to install terraform-config-inspect
- users can change the version of terraform-config-inspect based on the version of Terraform
  - I'm not familiar with the compatibility of Terraform and terraform-config-inspect, but we don't care about it
cons
- Users have to install terraform-config-inspect themselves
- User have to update terraform-config-inspect themselves
- Maybe some users keep using old terraform-config-inspect and we have to take care of old terraform-config-inspect

suzuki-shunsuke · 2024-03-16T05:10:25Z

suzuki-shunsuke Replaced Vitest with Jest

Thank you so much!

suzuki-shunsuke · 2024-03-16T05:52:37Z

add local-path-module tfaction-docs#224

suzuki-shunsuke · 2024-03-16T05:55:41Z

v1.3.0 is out 🎉
https://github.com/suzuki-shunsuke/tfaction/releases/tag/v1.3.0

Target module callers when module is changed

240f2ac

suzuki-shunsuke self-requested a review February 13, 2024 09:22

suzuki-shunsuke reviewed Feb 13, 2024

View reviewed changes

Put the new behavior behind the flag

246a10d

suzuki-shunsuke reviewed Feb 17, 2024

View reviewed changes

exoego added 4 commits February 17, 2024 15:23

Don't install when disabled

ee23e6f

Pin to the stable version

a7d4635

Rename to update_local_path_module_caller and disable by default

f6d9d8b

Call getInput in main

e150731

exoego force-pushed the update-module-use-site branch from d061624 to e150731 Compare February 17, 2024 06:41

exoego added 4 commits February 17, 2024 15:48

Declare Golang dependency

1aba475

Detect go bin path

3acf082

Move to the right place since it depends on global-config

922976c

last slash not needed

5ab2fad

suzuki-shunsuke added 2 commits February 17, 2024 17:04

style: format by prettier

c5edbf9

Merge remote-tracking branch 'origin/main' into update-module-use-site

6673ec7

Merge branch 'main' into update-module-use-site

1b02f65

suzuki-shunsuke added the enhancement New feature or request label Mar 14, 2024

Merge branch 'main' into update-module-use-site

0a33035

suzuki-shunsuke reviewed Mar 16, 2024

View reviewed changes

list-module-callers/package.json Outdated Show resolved Hide resolved

fix(list-module-caller): specify the install path of terraform-config…

8d83b3e

…-inspect

suzuki-shunsuke and others added 6 commits March 16, 2024 11:56

fix(list-targets): fix typo

ddf2cdd

fix(list-targets): change the install path of terraform-config-inspec…

f0c38d5

…t to a temporal directory It seems that action can't refer files on other actions

fix: stop installing terraform-config-inspect

5aea5b6

You have to install terraform-config-inspect yourself

fix: remove an input terraform_config_inspect_path

affbf63

Use jest

8b1f1ca

Merge remote-tracking branch 'origin/update-module-use-site' into upd…

914aaed

…ate-module-use-site # Conflicts: # list-module-callers/test/lib.test.ts

suzuki-shunsuke merged commit 6a5ff89 into suzuki-shunsuke:main Mar 16, 2024
12 checks passed

suzuki-shunsuke added this to the v1.3.0 milestone Mar 16, 2024

exoego deleted the update-module-use-site branch March 16, 2024 07:22

suzuki-shunsuke mentioned this pull request Mar 16, 2024

Migrate to vitest #1590

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Target module callers when local path module is updated #1528

Target module callers when local path module is updated #1528

exoego commented Feb 13, 2024 •

edited

Loading

suzuki-shunsuke left a comment •

edited

Loading

suzuki-shunsuke commented Feb 13, 2024

exoego commented Feb 13, 2024

suzuki-shunsuke commented Feb 13, 2024

exoego commented Feb 14, 2024

suzuki-shunsuke Feb 17, 2024

exoego Feb 17, 2024

exoego Feb 17, 2024

suzuki-shunsuke Feb 17, 2024

exoego Feb 17, 2024

suzuki-shunsuke Feb 17, 2024

exoego Feb 17, 2024

suzuki-shunsuke commented Feb 17, 2024

suzuki-shunsuke Feb 17, 2024

exoego Feb 17, 2024

exoego commented Feb 17, 2024

suzuki-shunsuke commented Feb 17, 2024

exoego commented Mar 12, 2024

suzuki-shunsuke Mar 16, 2024

exoego Mar 16, 2024

suzuki-shunsuke Mar 16, 2024

suzuki-shunsuke commented Mar 16, 2024

suzuki-shunsuke commented Mar 16, 2024 •

edited

Loading

exoego commented Mar 16, 2024

suzuki-shunsuke commented Mar 16, 2024 •

edited

Loading

suzuki-shunsuke commented Mar 16, 2024

suzuki-shunsuke commented Mar 16, 2024

suzuki-shunsuke commented Mar 16, 2024

Target module callers when local path module is updated #1528

Target module callers when local path module is updated #1528

Conversation

exoego commented Feb 13, 2024 • edited Loading

Issue

Solution

Implementation detail

suzuki-shunsuke left a comment • edited Loading

Choose a reason for hiding this comment

suzuki-shunsuke commented Feb 13, 2024

exoego commented Feb 13, 2024

suzuki-shunsuke commented Feb 13, 2024

exoego commented Feb 14, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suzuki-shunsuke commented Feb 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exoego commented Feb 17, 2024

suzuki-shunsuke commented Feb 17, 2024

exoego commented Mar 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suzuki-shunsuke commented Mar 16, 2024

suzuki-shunsuke commented Mar 16, 2024 • edited Loading

Release Note

Features

exoego commented Mar 16, 2024

suzuki-shunsuke commented Mar 16, 2024 • edited Loading

suzuki-shunsuke commented Mar 16, 2024

suzuki-shunsuke commented Mar 16, 2024

suzuki-shunsuke commented Mar 16, 2024

exoego commented Feb 13, 2024 •

edited

Loading

suzuki-shunsuke left a comment •

edited

Loading

suzuki-shunsuke commented Mar 16, 2024 •

edited

Loading

suzuki-shunsuke commented Mar 16, 2024 •

edited

Loading