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

Modules support relative path 2 #1726

Merged
merged 18 commits into from
Oct 17, 2024
Merged

Modules support relative path 2 #1726

merged 18 commits into from
Oct 17, 2024

Conversation

wildum
Copy link
Contributor

@wildum wildum commented Sep 23, 2024

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.

import.file “default” {
  filename = file.path_join(module_path, "relative/path")
}

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

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated

@wildum wildum requested a review from a team as a code owner September 23, 2024 11:17
@wildum wildum force-pushed the modules-support-relative-path-2 branch from 7bdd35b to 69b8022 Compare September 23, 2024 11:52
internal/runtime/alloy.go Outdated Show resolved Hide resolved
Copy link
Contributor

@thampiotr thampiotr left a 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.

@thampiotr
Copy link
Contributor

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.

Copy link
Contributor

@thampiotr thampiotr left a 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 Show resolved Hide resolved
internal/util/filepath.go Outdated Show resolved Hide resolved
@wildum
Copy link
Contributor Author

wildum commented Oct 2, 2024

Before merging, would it make sense to share it with someone who uses modules extensively to test this build?

@bentonam would be a good candidate

@wildum wildum requested a review from thampiotr October 11, 2024 14:16
Copy link
Contributor

@thampiotr thampiotr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see vm.Scope to have a constructor, but otherwise LGTM.

docs/sources/reference/config-blocks/import.file.md Outdated Show resolved Hide resolved
docs/sources/reference/config-blocks/import.git.md Outdated Show resolved Hide resolved
ComponentBlocks: source.components,
ConfigBlocks: source.configBlocks,
DeclareBlocks: source.declareBlocks,
ArgScope: &vm.Scope{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the rules for the vm.Scope now? Does it always need to have either a Parent or ModulePath set?

I think the way it's instantiated now is error-prone. I would prefer to have specialised constructors in vm package that enforce correct creation of vm.Scope so that we don't make mistakes or have semantic merge conflicts that result in bugs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added via d543652

@wildum wildum force-pushed the modules-support-relative-path-2 branch from f2c018c to 72440bd Compare October 16, 2024 10:57
Copy link
Contributor

@clayton-cornell clayton-cornell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK in a local build. I may open a new PR to change the way we present the main.alloy , lib.alloy, etc. code blocks, but... it's good enough as it is right now for this PR.

@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Oct 16, 2024
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
Copy link
Contributor

@thampiotr thampiotr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wildum wildum merged commit b92ac52 into main Oct 17, 2024
17 of 18 checks passed
@wildum wildum deleted the modules-support-relative-path-2 branch October 17, 2024 12:02
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age type/docs Docs Squad label across all Grafana Labs repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modules: support relative path
3 participants