Conversation
Merge telegraf master
config/config.go
Outdated
| urlfile := os.Getenv("TELEGRAF_CONFIG_URL") | ||
| envfile := os.Getenv("TELEGRAF_CONFIG_PATH") |
There was a problem hiding this comment.
The -config command line option takes either a local path or a URL. I think we should also have one environment variable that takes either a local path or a URL. I would prefer to keep TELEGRAF_CONFIG_PATH for backward compatibility and allow it to accept URLs.
It is unfortunate that the "_PATH" might make users think it only accepts local paths, but I think this is better than having two environment variables. We should make sure to add to the documentation to let users know URLs work too.
In the long term we may want to consider renaming the single env var to something more generic like "TELEGRAF_CONFIG" but that would be a breaking change so it would have to wait for a major release (2.0.0) where compatibility can be broken.
For this PR to be merged, could you remove the new TELEGRAF_CONFIG_URL env var? It looks like we may also need to combine the getDefaultConfigPath and LoadConfig and loadConfig functions because getDefaultConfigPath checks if the string is a file path, but loadConfig checks if it's a URL. I think we will need to do that in the same function.
There was a problem hiding this comment.
@Kuzyashin What do you think of this approach? Are you able to work on this PR more?
There was a problem hiding this comment.
Yes. I can push new commits in 2-3 days.
There was a problem hiding this comment.
Let me know if you can't find the time for this and we'll pass it on to someone else.
There was a problem hiding this comment.
@reimda can you please check again, I think your comment was resolved with the latest commits.
|
@Kuzyashin did you have any chance to take a look? |
srebhan
left a comment
There was a problem hiding this comment.
Hey @Kuzyashin! Thanks for your quick response. There are two minor comment in the code. Can you please take a look?
Yes. Sorry for the long answer. Lot of work lately. |
srebhan
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks again for the quick cycle!
|
@Kuzyashin looks like you need to resolve the conflict... :-) |
|
@Kuzyashin the majority of the CI tests failed. Can you please take a look and fix the problem if there is any? Edit: There seems to be a close-bracket missing in the test. :-) |
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
Closes #8909.
Add $TELEGRAF_CONFIG_URL to config discovery