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

Look up config file in default locations #6160

Merged
merged 5 commits into from
Jun 3, 2022

Conversation

chaudum
Copy link
Contributor

@chaudum chaudum commented May 13, 2022

What this PR does / why we need it:

This change allows to start Loki without -config.file argument, by making it optional and by default look up the config file in the current directory or in a config/ sub-directory.

This makes first use of Loki even simpler, especially when we distribute a tarball/zip that contains both the binary and a config file for use with local filesystem. Then Loki can be invoked simply with ./bin/loki within the extracted folder.

Pr-requisite for #6159

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated
  • Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

Copy link
Contributor

@kavirajk kavirajk left a comment

Choose a reason for hiding this comment

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

LGTM. Added one small suggestion to make error more useful.

return YAML(val, expandEnv)(dst)
}
}
return fmt.Errorf("%s does not exist", f.Value.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("%s does not exist", f.Value.String())
return fmt.Errorf("%s config does not exist, set %s for custom config path", f.Value.String(), name)

Copy link
Contributor

Choose a reason for hiding this comment

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

Added this suggestion because, currently it just prints
config.yaml does not exist if expected config file not found. Better to give some context where this is coming from and how to change it.

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

I think this is pretty reasonable, but want to let others dissent before we merge (cc @cyriltovena @slim-bean). Also, can we add some docs describing the new behavior?

@cstyan
Copy link
Contributor

cstyan commented May 16, 2022

I think having a default location for the config file makes sense

@chaudum chaudum force-pushed the chaudum/default-config-file-location branch from e627945 to 73594d8 Compare May 17, 2022 06:20
@chaudum chaudum marked this pull request as ready for review May 17, 2022 06:24
@chaudum chaudum requested review from KMiller-Grafana and a team as code owners May 17, 2022 06:24
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

1 similar comment
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

Copy link
Contributor

@kavirajk kavirajk left a comment

Choose a reason for hiding this comment

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

Nice addition :). Would wait what @slim-bean think before merging it though.

@chaudum chaudum force-pushed the chaudum/default-config-file-location branch from de0a69f to ff442d1 Compare May 30, 2022 16:24
chaudum added 5 commits June 1, 2022 11:05
This change makes Loki look for a config file in the current directory
or in the config/ sub-directory.

This is a pre-requisite for making an easy-to-use tarball distribution
that also ships a config.yaml

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
The `YAMLFlag` function is solely used for loading config files.

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
…ectories

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
@chaudum chaudum force-pushed the chaudum/default-config-file-location branch from ff442d1 to d64dac0 Compare June 1, 2022 09:07
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

Copy link
Contributor

@vlad-diachenko vlad-diachenko left a comment

Choose a reason for hiding this comment

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

LGTM

@owen-d owen-d merged commit 9a82f5c into main Jun 3, 2022
@owen-d owen-d deleted the chaudum/default-config-file-location branch June 3, 2022 12:13
@osg-grafana osg-grafana added type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories and removed area/docs labels Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants