-
Notifications
You must be signed in to change notification settings - Fork 235
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
add support for 'git::' force support on local filepaths, both absolute and relative #268
Comments
This was referenced Aug 28, 2020
salewski
added a commit
to salewski/go-getter
that referenced
this issue
Aug 31, 2020
The existing Detector interface cannot be extended in a backward compatible way to support new features desired for the GitDetector implementation[0]. The new CtxDetector interface is introduced to allow such extension without breaking backward compatibility with existing users. The new interface also avoids adding new methods to the existing Detector interface that would then need to be supported by all Detector implementations, both in-library and in the wild. A CtxDetector is slightly more cumbersome to use than Detector. Callers can (and should) continue to use Detector unless the enhanced capabilities of one or more of the CtxDetector implementation modules is needed. At the time of writing (2020-08), the only CtxDetector with such extra mojo is the forthcoming GitCtxDetector. Existing Detector implementations can easily be wrapped by CtxDetector implementations. The information available to a CtxDetector impl. is a strict superset of the information provided to a Detector. Where there is no need for the additional context info provided by the CtxDetect dispatch function, impls. can simply pass through the common subset to the Detect method of the analogous Detector impl. CAVEAT: In this changeset the list of ContextualDetectors is commented- out. This is intended to make a clear introduction of the interface type prior to introducing any implementations of it. A forthcoming change will provide such wrapping for all in-tree Detector impls, followed by the introduction of specialization for the GitCtxDetector impl. [0] C.f., hashicorp#268
salewski
added a commit
to salewski/terraform
that referenced
this issue
Aug 31, 2020
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
salewski
added a commit
to salewski/go-getter
that referenced
this issue
Jun 5, 2023
The existing Detector interface cannot be extended in a backward compatible way to support new features desired for the GitDetector implementation[0]. The new CtxDetector interface is introduced to allow such extension without breaking backward compatibility with existing users. The new interface also avoids adding new methods to the existing Detector interface that would then need to be supported by all Detector implementations, both in-library and in the wild. A CtxDetector is slightly more cumbersome to use than Detector. Callers can (and should) continue to use Detector unless the enhanced capabilities of one or more of the CtxDetector implementation modules is needed. At the time of writing (2020-08), the only CtxDetector with such extra mojo is the forthcoming GitCtxDetector. Existing Detector implementations can easily be wrapped by CtxDetector implementations. The information available to a CtxDetector impl. is a strict superset of the information provided to a Detector. Where there is no need for the additional context info provided by the CtxDetect dispatch function, impls. can simply pass through the common subset to the Detect method of the analogous Detector impl. CAVEAT: In this changeset the list of ContextualDetectors is commented- out. This is intended to make a clear introduction of the interface type prior to introducing any implementations of it. A forthcoming change will provide such wrapping for all in-tree Detector impls, followed by the introduction of specialization for the GitCtxDetector impl. [0] C.f., hashicorp#268
salewski
added a commit
to salewski/go-getter
that referenced
this issue
Jun 6, 2023
The existing Detector interface cannot be extended in a backward compatible way to support new features desired for the GitDetector implementation[0]. The new CtxDetector interface is introduced to allow such extension without breaking backward compatibility with existing users. The new interface also avoids adding new methods to the existing Detector interface that would then need to be supported by all Detector implementations, both in-library and in the wild. A CtxDetector is slightly more cumbersome to use than Detector. Callers can (and should) continue to use Detector unless the enhanced capabilities of one or more of the CtxDetector implementation modules is needed. At the time of writing (2020-08), the only CtxDetector with such extra mojo is the forthcoming GitCtxDetector. Existing Detector implementations can easily be wrapped by CtxDetector implementations. The information available to a CtxDetector impl. is a strict superset of the information provided to a Detector. Where there is no need for the additional context info provided by the CtxDetect dispatch function, impls. can simply pass through the common subset to the Detect method of the analogous Detector impl. CAVEAT: In this changeset the list of ContextualDetectors is commented- out. This is intended to make a clear introduction of the interface type prior to introducing any implementations of it. A forthcoming change will provide such wrapping for all in-tree Detector impls, followed by the introduction of specialization for the GitCtxDetector impl. [0] C.f., hashicorp#268 Signed-off-by: Alan D. Salewski <ads@salewski.email>
Just playing around today, and this syntax actually works already: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
It would be cool if the
git::
forcing token could be used on local file system paths, both absolute paths and relative paths. For example:or:
Internally,
go-getter
should transform the provided string into afile://
URI with an absolute filepath, with query string params and subdirectory notation retained.The rationale for using a
file://
URI internally is that the Git clone operation can work withfile://
URIs, and using them for this feature would allow us to leverage the existinggo-getter
URI-handling machinery. That would get us support for query params (to clone a specific git ref (tag, commit hash, ...)) "for free".The rationale for using an absolute filepath (even when the provided string is a relative filepath) is that (per RFC 1738 and RFC 8089) only absolute filepaths are legitimate in
file://
URIs. But more importantly, the Git clone operation only supportsfile://
URIs with absolute paths.Q: Why support this functionality at all?
Why not just require that a source location use an absolute path in a
file://
URI explicitly if that's what is needed?A: The primary reason is to allow support for relative filepaths to Git repos.
There are use cases in which the absolute path cannot be known in advance, but a relative path to a Git repo is known.
For example, when a Terraform project (or any Git-based project) uses Git submodules, it will know the relative location of the Git submodule repos, but cannot know the absolute path in advance because it will vary based on where the "superproject" repo is cloned. Nevertheless, those relative paths should be usable as clonable Git repos, and this mechanism would allow for that.
Support for filepaths that are already absolute would be provided mainly for symmetry. It would be surprising for the feature to work with relative file paths, but not for absolute filepaths.
For projects using Terraform, in particular, this feature would enable the non-fragile use of relative paths in a
module
"call" block, when combined with Git submodules:In the above example "superproject" Git repo (the one "calling" the terraform module) knows the relative path to its own Git submodules because they are embedded in a subdirectory beneath the top-level of the "superproject" repo.
There are at least two downstream Terraform issues open that would require
go-getter
support for this feature:I have a branch that implements this feature. I'll open a PR to get feedback on it shortly.
The text was updated successfully, but these errors were encountered: