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

Promtool: add http config support to query commands #11487

Merged
merged 2 commits into from Feb 14, 2023

Conversation

FUSAKLA
Copy link
Contributor

@FUSAKLA FUSAKLA commented Oct 24, 2022

Hi,

as a followup to the previous PR from Alertmanager prometheus/alertmanager#2764 and stale PR for promtool #8082

This PR adds support for HTTP config using the promconfig common package same as amtool does for query subcommands.

Works just fine, tested with base auth and bearer token.

@roidelapluie would you mind taking a look since you were actibe in both above linked PRs? 🙏

@FUSAKLA FUSAKLA requested a review from dgl as a code owner October 24, 2022 23:29
@FUSAKLA FUSAKLA changed the title feat: add promtool http config support Promtool: add http config support to query commands Oct 29, 2022
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Nov 15, 2022

@dgl would you find a second to review please?:pray:

@roidelapluie
Copy link
Member

Can we move this code to Prometheus/common?

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Nov 15, 2022

Can we move this code to Prometheus/common?

Which part of the code do you exactly mean to move to the common? The amconfig.LoadHTTPConfigFile?

@roidelapluie
Copy link
Member

yes please

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Nov 26, 2022

Actually it appears the function is already there, just in the test files and not used, nor by Alertmanager or Prometheus itself.
Alertmanager has one additional line in that function, not sure hot to deal with this difference.

PR here prometheus/common#415

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Dec 9, 2022

Switched to prometheus/common as agreed after prometheus/common#415

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Dec 15, 2022

Rebased, @roidelapluie friendly bump if you would find awhile 🙏

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Jan 3, 2023

Rebased again

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Jan 9, 2023

Rebased, fuzzing seems to be flaky?

@bboreham
Copy link
Member

Fuzzing issue #11810

@bboreham
Copy link
Member

Would you consider including tsdb create-blocks-from rules, which also performs queries ?
(no pressure, just asking)

@Pokom
Copy link

Pokom commented Jan 27, 2023

I have a basic version of tsdb create-blocks-from rules working here: Pokom@94094bf. For the first pass I opted to get the code working where the flag isn't shared between the two subcommands.

If FUSAKLA isn't keen to implement the feature in this PR, I'd be more then happy to follow up with a PR to implement it.

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 6, 2023

Hi, sorry for delay. Sure, I'll add the support event for the tsdb create-blocks-from rules.

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 6, 2023

@bboreham PTAL

@roidelapluie friendly bump once again 🙏

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 6, 2023

The failing CI seems weird once again 🤔
The issues with go mod seems to be unrelated or at least unintentional.. whole make and tests passes locally

@roidelapluie
Copy link
Member

can you run go mod tidy?

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 7, 2023

I ran it multiple times locally (go 1.19) and in container (official 1.18) with no changes 🤔
Running whole make passes

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 7, 2023

I'm not able to reproduce anyhow locally the issues CI bumps into.
My changed does not change any dependencies, so I do not see reason for it to change to cause changes in go.mod or go.sum :/

@bboreham
Copy link
Member

bboreham commented Feb 7, 2023

I think there is some general issue with CI - I've seen these error messages on other PRs.
Didn't yet figure out what the issue is.

@FUSAKLA FUSAKLA force-pushed the fus-promtool-http-config branch 2 times, most recently from 673f76c to b3abb39 Compare February 7, 2023 16:13
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 7, 2023

I suspected bump in the golang-builder on Go 1.20, but that seems also not to be an issue… 🤔
Might be worth trying to run on main branch to see if it fails only on branches, seeing other PRs with the same issues as Bryan sated

Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 10, 2023

🎉 rebased on master and CI passes..

@roidelapluie Would you take a look 🙏

Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

lgtm

cmd/promtool/main.go Outdated Show resolved Hide resolved
@roidelapluie
Copy link
Member

Hi, sorry for delay. Sure, I'll add the support event for the tsdb create-blocks-from rules.

Does not seem to be done

Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 14, 2023

Should be fixed now 🤞

@roidelapluie roidelapluie merged commit 8a8f594 into prometheus:main Feb 14, 2023
@roidelapluie
Copy link
Member

Thanks!

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Feb 14, 2023

Thank you! 🎉

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.

None yet

4 participants