feat(manager/terragrunt): support lockFileMaintenance

[spilliams]

terragrunt configurations sometimes have no tf files, but do have terraform lockfiles.

Changes

Adds a function updateArtifacts to the Terragrunt manager that wraps the Terraform manager's implementation. As requested in this discussion, it only calls the downstream function if the update type is lockFileMaintenance.

Context

The current Renovate behavior is that it normally does not update a Terraform lockfile if it is in a directory with no .tf files. This can be the case when the configuration is managed by Terragrunt, where the terragrunt.hcl includes a terraform block to call out to an external module.

The expected behavior is that when Renovate runs on a minimal Terragrunt configuration, it will update the versions and hashes stored in the .terraform.lock.hcl. This is a listing of what a minimal Terragrunt configuration contains:

my-service
├── .terraform.lock.hcl
├── .terragrunt-cache/
└── terragrunt.hcl

and a minimal terragrunt.hcl is:

terraform {
  source = "path/to/module"
}

Closes #13393

Documentation (please check one with an [x])

• I have updated the documentation, or
• No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

• Code inspection only, or
• Newly added/modified unit tests, or
• No unit tests but ran on a real repository, or
• Both unit tests + ran on a real repository

My test repository is here. The "before" PR was this one and the "after" PR was this one.

I've run yarn test locally and it shows that coverage is unchanged from the before this branch:

=============================== Coverage summary ===============================
Statements   : 99.99% ( 33708/33709 )
Branches     : 98.08% ( 10843/11055 )
Functions    : 100% ( 4076/4076 )
Lines        : 99.99% ( 33217/33218 )
================================================================================

[viceice]

needs unit test

[spilliams]

needs unit test

@viceice What kind of unit test were you thinking? The logic in this function is straightforward enough to me that I didn't think it warrants one, but I'm also new to this codebase.

[spilliams]

When I run yarn test from my fork's main branch (up to date with upstream remote), I get 2 failed tests and less than 100% coverage:

=============================== Coverage summary ===============================
Statements   : 99.99% ( 33695/33696 )
Branches     : 98.08% ( 10838/11050 )
Functions    : 100% ( 4074/4074 )
Lines        : 99.99% ( 33205/33206 )
================================================================================
Jest: "global" coverage threshold for statements (100%) not met: 99.99%
Jest: "global" coverage threshold for lines (100%) not met: 99.99%

Summary of all failing tests
 FAIL  lib/util/github/graphql/cache-strategies/package-cache-strategy.spec.ts (170 MB heap size)
  ● util/github/graphql/cache-strategies/package-cache-strategy › reconciles old cache record with new items

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

    @@ -22,8 +22,8 @@
                "version": "4",
              },
            },
            "updatedAt": "2022-10-30T12:00:00.000-07:00",
          },
    -     21600,
    +     21660,
        ],
      ]

      46 |     expect(res).toEqual([...Object.values(oldItems), newItem]);
      47 |     expect(isPaginationDone).toBe(false);
    > 48 |     expect(cacheSet.mock.calls).toEqual([
         |                                 ^
      49 |       [
      50 |         'foo',
      51 |         'bar',

      at Object.<anonymous> (lib/util/github/graphql/cache-strategies/package-cache-strategy.spec.ts:48:33)

 FAIL  lib/modules/platform/github/index.spec.ts (22.313 s, 550 MB heap size)
  ● modules/platform/github/index › getBranchPr(branchName) › aborts reopen if PR is too old

    Missing mocks!
     *   POST https://api.github.com/repos/some/repo/git/refs

      51 |   missingLog = [];
      52 |   if (missing.length && throwOnPending) {
    > 53 |     throw new Error(`Missing mocks!\n * ${missing.join('\n * ')}`);
         |           ^
      54 |   }
      55 |   if (!isDone && throwOnPending) {
      56 |     throw new Error(`Pending mocks!\n * ${pending.join('\n * ')}`);

      at clear (test/http-mock.ts:53:11)
      at Object.<anonymous> (test/http-mock.ts:120:3)


Test Suites: 2 failed, 590 passed, 592 total
Tests:       2 failed, 11905 passed, 11907 total
Snapshots:   864 passed, 864 total
Time:        233.747 s
Ran all test suites.

When I run yarn test from the development branch, I get the same two failures and a slightly worse coverage:

=============================== Coverage summary ===============================
Statements   : 99.98% ( 33700/33704 )
Branches     : 98.07% ( 10838/11051 )
Functions    : 99.97% ( 4075/4076 )
Lines        : 99.98% ( 33209/33213 )
================================================================================

I'll update my branch to try and at least get the Functions coverage back to 100%

UPDATE: as of ee132c9, coverage levels are back to the original:

=============================== Coverage summary ===============================
Statements   : 99.99% ( 33708/33709 )
Branches     : 98.08% ( 10843/11055 )
Functions    : 100% ( 4076/4076 )
Lines        : 99.99% ( 33217/33218 )
================================================================================

[viceice]

did you test the latest version against real repos? I'd like to see some working sample PR for different update types.

[spilliams]

did you test the latest version against real repos?

@viceice Yes, I tested on my public repository here. I've summarized all tests to date in this wiki page

[viceice]

did you test the latest version against real repos?

@viceice Yes, I tested on my public repository here. I've summarized all tests to date in this wiki page

Idon't see a pr open with a changed lockfile in your test rep 👀

[spilliams]

Idon't see a pr open with a changed lockfile in your test rep 👀

That's because I close the PRs as I go, to provide a clean testing workbench (when I run yarn start ... it might create multiple PRs at once). See all PRs in that repo here.

[spilliams]

I just ran it again, from commit 5fa37874f, and left the resulting PRs open for visibility (required_provider minor and lockfile).

[viceice]

as you can see, the provider PR is missing a lockfile update. so it needs more things to do here to support that case.

maybe we simply need to match the extract properties to terraform.

@secustor can you help?

[secustor]

🤔 for this to work we need to follow the reference to the modules and then do a full extraction with the terraform manager. This will give us the updated dependencies which we than can use to update the lock file.

The problem with that, besides that it is costly, is that we have to leave the directory where the package file is located.
We forbid this currently in most cases because of security concerns. Further we would have to inject updates of other managers ( terraform in this case ) to updateArtifacts.
@rarkins @viceice WDYT regarding this?

[rarkins]

The problem with that, besides that it is costly, is that we have to leave the directory where the package file is located. We forbid this currently in most cases because of security concerns.

Leave it to where? It's ok to traverse the repo itself

Further we would have to inject updates of other managers ( terraform in this case ) to updateArtifacts. @rarkins @viceice WDYT regarding this?

What type of updates? Do you mean run multiple binaries?

[secustor]

Leave it to where? It's ok to traverse the repo itself

No still inside the repo but I don't think we have function to do find file patterns safely. We have one if we know the file names tough.

What type of updates? Do you mean run multiple binaries?

The resulting updates from the terraform manager before hitting the updateArtifacts phase.

[spilliams]

Hi all,

It sounds like it would be difficult, or at least lengthy and complex, to implement "lockfile updates as part of module updates". I shouldn't have brought it up in the first place, considering it's not the main focus of #13393 .

In order to keep the scope of this PR contained I'd like to focus on the current implementation, which provides a solution to "terragrunt configurations don't get lockfile updates". I believe the code is sound, and my tests show that this branch does exhibit the desired behavior.

The last thing remaining for me to do (that I know of) is to set up an end-to-end test in lib/modules/manager/terragrunt/index.spec.ts, testing this behavior change.

Am I missing anything? How does the current implementation look to you?

Thank you all for helping me with this, it will make a big difference to my team's weekly operations.

[secustor]

You need unit tests which call the updateArtifacts of the terragrunt manager with updateType set to lockfileMaintenance. If the updateType is set to something different, currently updateArtifact should return null.

At best you implement updateArtifacts in terragrunt, check for updateType and then return null or the result of the terraform manager.
You can take a look how the terraform manager tests lockfileMaintenance here:

renovate/lib/modules/manager/terraform/lockfile/index.spec.ts

Line 317 in 39493b7

it('do full lock file maintenance', async () => {

[relsqui]

Hi, I'm a teammate of @spilliams's taking a look at this while he's focused on other work. Am I understanding these requested changes correctly?

• rather than just re-exporting the Terraform function from the Terragrunt manager, put it in a small wrapper which returns null if updateType isn't lockFileMaintenance (since we're limiting the scope to just that update type, and the Terraform function still does other things in that case)
• add a test that verifies that the wrapper returns null when updateType isn't lockFileMaintenance
• add a test that verifies the wrapper does the same thing as the Terraform lock file updater when the updateType is lockFileMaintenance. I read through the existing test you linked and, while my Jest is pretty rusty, I didn't notice anything that would be different if we reimplemented it for the Terragrunt manager. is it still useful to copy that code over so it can run from the Terragrunt context as well?

[rarkins]

If a terragrunt file has a lock file, shouldn't the lock file be updated any time the package file is updated?

If I understand this PR, it would only update this lock file independently on a lock file maintenance schedule, while all other PRs would have an un-updated and hence out of date lock file?

[rarkins]

If a terragrunt file has a lock file, shouldn't the lock file be updated any time the package file is updated?

If I understand this PR, it would only update this lock file independently on a lock file maintenance schedule, while all other PRs would have an un-updated and hence out of date lock file?

This remains unanswered?

[spilliams]

Thanks for pinging that @rarkins. Here are my thoughts, but I believe @secustor has some thoughts as well:

If a terragrunt file has a lock file, shouldn't the lock file be updated any time the package file is updated?

Terragrunt (and Terraform) deployments don't always have package files. There may be updated constraints on dependency versions, but in general for my use cases I keep my coded dependency constraints as low as possible and try to keep my lockfile updated with the latest versions of my dependencies. This usually means running terraform init -upgrade locally.

If I understand this PR, it would only update this lock file independently on a lock file maintenance schedule, while all other PRs would have an un-updated and hence out of date lock file?

Perhaps. I'm not really sure since I'm so new to the inner workings of Renovate. I'll say two things here:

1. As a Terraform/Terragrunt user, I expect any tool that works on Terraform to behave identically with a Terragrunt implementation. This is why I proposed early in this PR that the terragrunt manager wholly import the terraform manager's updateArtifacts function.
2. Sebastian had recommended both in this PR and in the prior discussion that we use a wrapper function that only updates lockfiles under certain conditions. I admit I don't fully understand the logic behind this recommendation, but I deferred to their knowledge of the system and implemented the solution as requested.

[rarkins]

@secustor can you clarify why you think only lock file maintenance is necessary?

[secustor]

I think it is necessary, but requires larger changes in the code base to work efficiently outside of lockFileMaintenance as we have to rerun the terraform manager for specific directories. This could be solved by caching based on the directories.

I see lockFileMaintenance as good first step to get basic support out and gauge if there is an interest to extend the manager.

Outdated

[rarkins]

@secustor ok, please merge then if you're happy with it

[renovate-release]

🎉 This PR is included in version 35.95.0 🎉

The release is available on:

• GitHub release
• 35.95.0

Your semantic-release bot 📦🚀

[relsqui]

Thank you maintainers! We just confirmed this works on our huge Terragrunt repo, and it's going to save us so much time.