-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Remove v1 client lib #2763
Remove v1 client lib #2763
Conversation
24b6cc9
to
effada6
Compare
Erk... This seems to be more ingrained than I anticipated. Not sure whether we want to do this @roidelapluie considering this might have a bit of an impact |
This is indeed used in the acceptance tests. What about we move client and client_test.go inside test/with_api_v1/acceptance.go and merge them into a file called helper_test.go ? test/with_api_v1/acceptance.go |
Sounds reasonable. The source of this confusion comes from the prominence of this lib in the repo, so moving this out of that location makes sense |
effada6
to
f507e12
Compare
Build error looks transient?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think only the client part shold go to helper.go, acceptance should stay in acceptance.go
96e4575
to
d182c98
Compare
This commit moves the stuff formerly in /client into /test/with_api_v1 so that we can discourage use of the v1 client without breaking things Signed-off-by: sinkingpoint <colin@quirl.co.nz>
d182c98
to
96bc0fb
Compare
Fixed this. Yet again, the build failures appear to be docker pull timeouts which is weird |
Thanks! |
This is confusing as it sits in the root of the repo. Considering we
have a v2 client lib at (https://github.com/prometheus/alertmanager/blob/main/api/v2/client/alertmanager_client.go) this commit removes the v1 one to try and avoid
that confusion
Solves #2762