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.

Sign up for GitHub

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

Jump to bottom

feat(flow): allow loading cfg srcs from dir #5228

Merged
merged 7 commits into from
Sep 21, 2023
Merged

feat(flow): allow loading cfg srcs from dir #5228

merged 7 commits into from
Sep 21, 2023

Conversation

hainenber
Copy link
Contributor

@hainenber hainenber commented Sep 17, 2023 •
edited
Loading

PR Description

Which issue(s) this PR implement

Implements #3855

Notes to the Reviewer

Overall, a port from @rfratto's closed PR with additional changes to address existing reviews

  • New tests to validate ParseSources in both happy and sad cases (addressed Matt review)
  • Explicitly document acceptance of both directory and file paths for River configs (addressed Erik's reviews)

Other reviews are valid to address but the PR is quite big to address them all atm. I'll try to address them in upcoming PRs

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated

@rfratto
Copy link
Member

rfratto commented Sep 18, 2023

Thanks for taking this over 🎉! I'll spend time reviewing this during working hours on Monday.

rfratto
rfratto reviewed Sep 19, 2023
View reviewed changes
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

Thanks! I double checked the original PR to make sure all the original comments are addressed.

I'd like to see a little bit more detail added to the docs, and then I'll approve and hand it over to Clayton for documentation review before we get this merged :)

component/module/module.go Outdated Show resolved Hide resolved
docs/sources/flow/reference/cli/run.md
Comment on lines +31 to 32
If the `PATH_NAME` argument is not provided, or if the configuration path can't be loaded or
contains errors during the initial load, the `run` command will immediately exit and show an error message.
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a new paragraph after this explaining the behavior if a folder is given?

@rfratto
Copy link
Member

rfratto commented Sep 19, 2023

There seems to be a lint error that also needs to be fixed, and one test is failing for an unrelated reason to this PR, but I think merging main in should fix that.

@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Sep 19, 2023
@rfratto rfratto requested a review from a team as a code owner September 21, 2023 15:20
@rfratto
Copy link
Member

rfratto commented Sep 21, 2023 •
edited
Loading

one test is failing for an unrelated reason to this PR, but I think merging main in should fix that.

@hainenber I don't know why this PR triggers that failure in an unrelated package, but I pushed code to fix it for you so we can move forward :)

@hainenber
Copy link
Contributor Author

Thank you @rfratto! That was niceee 😄

@hainenber @clayton-cornell
Update run.md
a93c258 
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
@clayton-cornell
Copy link
Contributor

Docs are OK as-is. There's still that one open doc question from Robert.... has it been resolved?

@hainenber
Copy link
Contributor Author

Docs are OK as-is. There's still that one open doc question from Robert.... has it been resolved?

Yea, I've pushed this commit to address it :D

@clayton-cornell
Copy link
Contributor

All good then. Over to @rfratto for final approval.

rfratto
rfratto approved these changes Sep 21, 2023
View reviewed changes
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

Thanks, super exciting to have this merged!

@rfratto rfratto merged commit 0b0b4a5 into grafana:main Sep 21, 2023
6 checks passed
hainenber added a commit to hainenber/agent that referenced this pull request Sep 23, 2023
@hainenber @rfratto @clayton-cornell
feat(flow): allow loading cfg srcs from dir (grafana#5228)
92d541e 
Co-authored-by: Robert Fratto <robertfratto@gmail.com>
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
@tpaschalis tpaschalis mentioned this pull request Oct 6, 2023
Closed
@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 22, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@clayton-cornell clayton-cornell clayton-cornell left review comments

@rfratto rfratto rfratto approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hainenber @rfratto @clayton-cornell