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

Wire up support for converting integrations-next configs to flow. #5973

Merged
merged 13 commits into from
Dec 15, 2023

Conversation

erikbaranowski
Copy link
Contributor

@erikbaranowski erikbaranowski commented Dec 13, 2023

Include a mechanism for passing command line flags to the converter from the old format.

PR Description

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

Include a mechanism for passing command line flags to the converter
from the old format.

Signed-off-by: erikbaranowski <[email protected]>
@@ -31,7 +31,7 @@ func Convert(in []byte, extraArgs []string) ([]byte, diag.Diagnostics) {
var diags diag.Diagnostics

fs := flag.NewFlagSet("convert", flag.ContinueOnError)
args := []string{"-config.file", "convert", "-config.expand-env"}
args := []string{"-config.file", "convert"}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove this hard coded since we can now pass this in and have more control over the converter. This was the lesser of 2 evils before to automatically expand environment vars.

@@ -1,3 +1,7 @@
server:
log_level: ${SOME_ENVIRONMENT_VARIABLE:='debug'}
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 an integration-next test for env vars to validate it also works.

@@ -10,8 +10,8 @@ import (
)

func TestConvert(t *testing.T) {
test_common.TestDirectory(t, "testdata", ".yaml", true, []string{}, staticconvert.Convert)
test_common.TestDirectory(t, "testdata-v2", ".yaml", true, []string{"-enable-features", "integrations-next"}, staticconvert.Convert)
test_common.TestDirectory(t, "testdata", ".yaml", true, []string{"-config.expand-env"}, staticconvert.Convert)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer hard coded to be added so we need to pass it in for the tests.

cmd/internal/flowmode/cmd_convert.go Outdated Show resolved Hide resolved
docs/sources/flow/getting-started/migrating-from-static.md Outdated Show resolved Hide resolved
cmd/internal/flowmode/cmd_run.go Outdated Show resolved Hide resolved
cmd/internal/flowmode/cmd_convert.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@mattdurham mattdurham left a comment

Choose a reason for hiding this comment

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

LGTM

docs/sources/flow/getting-started/migrating-from-static.md Outdated Show resolved Hide resolved
docs/sources/flow/getting-started/migrating-from-static.md Outdated Show resolved Hide resolved
docs/sources/flow/getting-started/migrating-from-static.md Outdated Show resolved Hide resolved
docs/sources/flow/reference/cli/convert.md Outdated Show resolved Hide resolved
docs/sources/flow/reference/cli/convert.md Outdated Show resolved Hide resolved
docs/sources/flow/reference/cli/convert.md Outdated Show resolved Hide resolved
docs/sources/flow/reference/cli/run.md Outdated Show resolved Hide resolved
@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Dec 14, 2023
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.

Nice work, thanks!

@@ -174,3 +187,35 @@ func supportedFormatsList() string {
}
return strings.Join(ret, ", ")
}

func parseExtraArgs(extraArgs string) ([]string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thanks for addressing this! I'd only add maybe a couple of comments, e.g. the two cases in for loop for --flag and -flag. Some test cases for failed parses or the --key="foo=bar" may be a good idea too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

once I did some deeper testing, I made some more significant improvements to the func. I added comments throughout due to the complexity which will hopefully help future us

docs/sources/flow/getting-started/migrating-from-static.md Outdated Show resolved Hide resolved
@erikbaranowski erikbaranowski enabled auto-merge (squash) December 15, 2023 19:42
@erikbaranowski erikbaranowski merged commit 078e736 into main Dec 15, 2023
10 checks passed
@erikbaranowski erikbaranowski deleted the converter-integrations-next-cli branch December 15, 2023 19:45
BarunKGP pushed a commit to BarunKGP/grafana-agent that referenced this pull request Feb 20, 2024
…afana#5973)

* Wire up support for converting integrations-next configs to flow.

Include a mechanism for passing command line flags to the converter
from the old format.

Signed-off-by: erikbaranowski <[email protected]>

* Update cmd/internal/flowmode/cmd_convert.go

Co-authored-by: Piotr <[email protected]>

* Apply suggestions from code review

Co-authored-by: Clayton Cornell <[email protected]>

* Update docs/sources/flow/getting-started/migrating-from-static.md

Co-authored-by: Piotr <[email protected]>

---------

Signed-off-by: erikbaranowski <[email protected]>
Co-authored-by: Piotr <[email protected]>
Co-authored-by: Clayton Cornell <[email protected]>
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. type/docs Docs Squad label across all Grafana Labs repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants