-
Notifications
You must be signed in to change notification settings - Fork 187
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
Modules support relative path 2 #1726
base: main
Are you sure you want to change the base?
Conversation
7bdd35b
to
69b8022
Compare
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this go through a proposal as a user-facing API?
I think there are different ways to design this that we'd want to consider before implementing.
Another idea, which I think could be powerful and simple for users to work with: if a file has a relative path in it, it is relative to that file's location. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small comments, but LGTM in general.
Before merging, would it make sense to share it with someone who uses modules extensively to test this build? Or check if it works with fleet management?
internal/runtime/alloy_services.go
Outdated
@@ -98,7 +98,8 @@ func (sc ServiceController) LoadSource(b []byte, args map[string]any) error { | |||
if err != nil { | |||
return err | |||
} | |||
return sc.f.LoadSource(source, args) | |||
// The config is loaded via a service, the config path is left empty. | |||
return sc.f.LoadSource(source, args, "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I use module_path
in such config, what path would it be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added support for it and made sure that it works with fleet management.
I did the following manual tests:
- remote config with import.git that imports a module that has import.file with relative path
- remote config with import.file that imports a module that has an import.file with relative path (it's relative to the path of the config where the remotecfg component is, not super useful I guess but that works and it's good for consistency)
f9f5784
@@ -608,7 +614,7 @@ func (l *Loader) wireGraphEdges(g *dag.Graph) diag.Diagnostics { | |||
} | |||
|
|||
// Finally, wire component references. | |||
refs, nodeDiags := ComponentReferences(n, g, l.log) | |||
refs, nodeDiags := ComponentReferences(n, g, l.log, l.cache.scope) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: We're reading l.cache.scope
without the mutex. It may be stale and come up in race detector, but I don't think it's dangerous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add mutex rlock and runlock to protect it: acf8ef6
func (im *ImportGit) ModulePath() string { | ||
return im.repoPath | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we now document that the entire repository will be cloned and files in the git repo will be available to reference them using module_path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added documentation: 133aeb0
internal/util/filepath.go
Outdated
// ExtractDirPath removes the file part of a path if it exists. | ||
func ExtractDirPath(p string) string { | ||
// If the base of the path has an extension, it's likely a file. | ||
if filepath.Ext(filepath.Base(p)) != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's likely that people will use a file without extension, e.g. /etc/alloy_config
and this will not work for them.
I think instead we should Stat
the path and check if it's directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did the change: 3f08a9e. The error should not happen because if the path is wrong then it will fail earlier but we use this func everywhere for testing so I only logged it with warn
@bentonam would be a good candidate |
@@ -41,6 +44,21 @@ The following arguments are supported: | |||
|
|||
This example imports a module from a file and instantiates a custom component from the import that adds two numbers: | |||
|
|||
{{< collapse title="main.alloy" >}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Description
This PR introduces an ArgScope to the loader that will be used as a parent scope when evaluating the nodes in the module. For now, it contains only one variable "module_path" that represents the current path of the module and can be used in the config. Both can be used together to configure a relative path on the import.file block.
The following use-cases are currently supported:
import.file in a module imported via import.file (module_path = filename of the parent import.file)
import.file in a module imported via import.git (module_path = path to the local cloned repo)
import.file in a module imported via import.string (module_path = path of the current module)
Which issue(s) this PR fixes
Fixes #786
Notes to the Reviewer
This was an old PR that I revived now that the stdlib has been updated.
I recommend reviewing commit by commit.
PR Checklist