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

Load config file from URL #1243

Merged
merged 5 commits into from
Jul 17, 2023
Merged

Conversation

DiniFarb
Copy link
Contributor

As requested in #1207 I've added support for loading the config file either from a directory or a URL.

@DiniFarb DiniFarb requested a review from a team as a code owner July 10, 2023 19:10
Copy link
Contributor

@breed808 breed808 left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this! Would it be possible to include a flag allowing a user to skip TLS verification for the config file? I know some users of the exporter may have self-signed certificates in place, despite it not being best practice.

Something like this, with insecure_skip_verify being passed to the NewResolver function as a bool parameter:

tr := &http.Transport{
        TLSClientConfig: &tls.Config{InsecureSkipVerify: insecure_skip_verify},
    }
    client := &http.Client{Transport: tr}
    _, err := client.Get(file)

@DiniFarb
Copy link
Contributor Author

DiniFarb commented Jul 10, 2023

Good point and it is good to have such a flag for testing purposes as well. I'll implement that.

The flag should then be passed as an argument like:

--config.file.skip-tls-verify=true right?

Or somehow passed in as a prefix or suffix in the url? Like:

--config.file="[insecure_skip_verify]https://example.net/config.yaml"

@breed808
Copy link
Contributor

Yes, a boolean flag would be preferable here (assuming the flag library supports that type).

@DiniFarb
Copy link
Contributor Author

Done, now its possible to skip TLS.
One more question though, I saw that the --config.file flag is not listed in the global flags section of the readme. Wouldn't it be good to add the config flag plus the new skip TLS flag well as?

@breed808
Copy link
Contributor

Sure, feel free to add it in the readme if you like.

One other thing: you'll need to sign-off the commit(s) in order to pass the CI DCO check. I can't recall if all commits need to be signed or just the last, but I'd assume the worst and sign all of them.

Let me know if you need assistance with that.

@DiniFarb
Copy link
Contributor Author

DiniFarb commented Jul 13, 2023

I'am not quite sure, should I use git rebase --signoff HEAD~6 to sign commits afterwarts ?

I have added the readme change directly online so the last commit is signed.

@breed808
Copy link
Contributor

Yes, the git rebase --signoff command should do it if the DCO check continues to fail

@breed808
Copy link
Contributor

Hmm, the force push seems to have added other commits, and there's been a couple merge commits since then.
Want some assistance rebasing the feature branch? I believe I can push to it.

@DiniFarb
Copy link
Contributor Author

Yes I have always synct the pr branch as soon as it was behind the prometheus-community:master. Shouldn't I have done that? 😬 sorry for that, and yes u should be able to push to the pr branch. Otherwise just write the rebase command I should use.

@breed808
Copy link
Contributor

Syncing it with the upstream master branch is generally advised, the issue here is that this PR is proposing to merge your master branch into the master branch of the windows_exporter project, so we'd end up merging merge commits. Merges all the way down 😄

I'll force-push to your master branch now in order to tidy the commits in this PR, once I've pushed have a review of the commits and let me know if you're happy with the result. If not I can revert the changes.

@breed808
Copy link
Contributor

Commits look alright now, though the DCO check is complaining again ☹️

Author: andreasvogt89, Committer: Ben Reedy; Expected "andreasvogt89 30302212+andreasvogt89@users.noreply.github.com", but got "Vogt Andreas, INI-MBM-BNC Andreas.Vogt3@swisscom.com"

@DiniFarb
Copy link
Contributor Author

DiniFarb commented Jul 17, 2023

yes my bad, I've used the wrong account last week to sign the commits 😐 I've done it again, but with the fork sync I've fu* up the commit timeline again. Could you help me with the rebase ?

Signed-off-by: DiniFarb <andreas.vogt89@bluewin.ch>
Signed-off-by: DiniFarb <andreas.vogt89@bluewin.ch>
Signed-off-by: DiniFarb <andreas.vogt89@bluewin.ch>
Signed-off-by: DiniFarb <andreas.vogt89@bluewin.ch>
Signed-off-by: DiniFarb <andreas.vogt89@bluewin.ch>
@breed808
Copy link
Contributor

Got it, the DCO check is happy now. Have a review of the commits and let me know if you're happy with them, then I'll get this merged.

@DiniFarb
Copy link
Contributor Author

I am very sorry for the confusion I've created here and many thx for fixing all 😊 The commits are looking very good and clean to me now 👍🏼

@breed808
Copy link
Contributor

It's fine, nothing to worry about. git is not the easiest tool to use, especially when trying to cleanly merge changes.

@breed808 breed808 merged commit 89cb543 into prometheus-community:master Jul 17, 2023
5 checks passed
@breed808 breed808 mentioned this pull request Jul 17, 2023
anubhavg-icpl pushed a commit to anubhavg-icpl/windows_exporter that referenced this pull request Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants