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

incorporate go-getter support for 'git::' filepaths #26056

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

salewski
Copy link
Contributor

This draft PR is a WIP, but please feel free to provide feedback.

The changes noted below require a WIP version of the go-getter project, currently available on the ads/git-forced-rel-path-to-abs branch of my GitHub fork of the project here:

The upstream project issue and draft PR for those changes is:

There has been some discussion of this feature on these terraform issues:


DRAFT DRAFT DRAFT DRAFT DRAFT DRAFT DRAFT


(Tentative PR cover letter follows)

The go-getters library now has support for local file system paths to Git repositories, specified with the git:: forcing token. The feature works for both absolute and relative filepaths, and supports all the usual go-getter goodies including // delimited subdirs and URI-style query parameters.

The featurs was introduced into the go-getter project with the following issue and PR for those changes is:

We incorporate that capability into Terraform, which allows users to specify paths to locally present Git repositories from which to clone other Terrform modules on which they are dependent. When coupled with Git submodules, this creates a powerful way to manage Terraform modules at specific versions without requiring those modules to be available on the network (e.g., on GitHub):

    module "my_module" {
        source = "git::../git-submodules/tf-modules/some-tf-module?ref=v0.1.0"
        // ...
    }

From the perspective of Terraform, such Git repositories are "remote" in the same way that repositories on GitHub are.

Note that within a Terraform module "call" block, the filepaths specified are relative to the directory in which the *.tf file lives, not relative to the current working directory of the Terraform process. (Thanks to @apparentlymart for pointing that out as one of the main challenges in this comment). In order to support this feature, Terraform needs to supply that contextual information to go-getter to allow relative filepath resolution to work. In order to do so, we needed to switch over to using go-getter's new "Contextual Detector" API. It works in the same basic way as the traditional Detector API, but allows us to provide this additional information.

In keeping with the "keep things simple" comment in the commit message of 2b2ac1f, we are here maintaining our custom go-getter detectors in two places. Only now each is called FooCtxDetector rather than FooDetector. Nevertheless, all except the GitCtxDetector do little more than "pass through" delegation to its analogous FooDetector counterpart.

Fixes #25488
Fixes #21107

[0] hashicorp/go-getter#268
[1] hashicorp/go-getter#269

The go-getters library now has support for local file system paths to
Git repositories, specified with the 'git::' forcing token. The feature
works for both absolute and relative filepaths, and supports all the
usual go-getter goodies including '//' delimited subdirs and URI-style
query parameters.[0][1]

We incorporate that capability into Terraform, which allows users to
specify paths to locally present Git repositories from which to clone
other Terrform modules on which they are dependent. When coupled with
Git submodules, this creates a powerful way to manage Terraform modules
at specific versions without requiring those modules to be available on
the network (e.g., on GitHub):

    module "my_module" {
        source = "git::../git-submodules/tf-modules/some-tf-module?ref=v0.1.0"
        // ...
    }

From the perspective of Terraform, such Git repositories are "remote" in
the same way that repositories on GitHub are.

Note that within a Terraform module "call" block, the filepaths
specified are relative to the directory in which the *.tf file lives,
not relative to the current working directory of the Terraform
process. In order to support this feature, Terraform needs to supply
that contextual information to go-getter to allow relative filepath
resolution to work. In order to do so, we needed to switch over to using
go-getter's new "Contextual Detector" API. It works in the same basic
way as the traditional Detector API, but allows us to provide this
additional information.

In keeping with the "keep things simple" comment in the commit message
of 2b2ac1f, we are here maintaining our custom go-getter detectors
in two places. Only now each is called FooCtxDetector rather than
FooDetector. Nevertheless, all except the GitCtxDetector do little more
than "pass through" delegation to its analogous FooDetector counterpart.

Fixes hashicorp#25488
Fixes hashicorp#21107

[0] hashicorp/go-getter#268
[1] hashicorp/go-getter#269
@yu-iskw
Copy link

yu-iskw commented May 24, 2021

@salewski Thank you for the PR. The feature is very useful to me. Is there any update?

@lae
Copy link

lae commented Jun 15, 2021

Also interested in this. Doesn't seem like the PR on go-getter has been merged yet, so that probably explains why this is still a draft.

@salewski
Copy link
Contributor Author

Hi @yu-iskw,
Hi @lae,

Back when I had momentum with this (August/September 2020), I had the feature working well and found it to be very useful -- in fact, indispensible for the (private) project on which I was working at the time. However, because of the above described implementation in the go-getter library, I felt like it was a big ask to push too hard for those "Contextual Detector" changes to be merged without very strong justification (e.g., in the form of a published project making use of them). I've had to direct my attention elsewhere, however, and haven't circled back on it yet.

IIRC, there are two potential issues:

  1. The (mostly) aesthetic issue that the CtxDetector API parallels the Detector API. As explained elsewhere I did not see how to implement the feature using the existing API without breaking backward compatibility (which I would not have proposed), so created a near-look-and-act-alike sibling API to do what I needed. It is more code to maintain, though, so the issue isn't only aesthetic.
  2. The "context" aspect of the new CtxDetect(...) function (that is, the addition parameters that it takes that the existing Detect(...) function does not) are rather specific to the needs of the GitCtxDetector. Other potential "context detectors" might need different contextual info, so maybe some more generic mechanism would be wanted -- not sure; might be overthinking it.

Your interest in the feature, though, has brought it back onto my radar (thanks for that). In the next two weeks (or so) I'll try to get my head back into it more fully and maybe find time to publish a small set of working demo projects that illustrate how I use it.

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants