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

cherry-pick bug fixes and cli patches into release-0.15 #1363

Closed
wants to merge 18 commits into from

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented May 5, 2018

I have cherry-picked all bug fixes as well as all CLI related patches into the release-0.15 branch. I would like to not include any breaking changes or new features between two release candidates. Thereby I have not included the following commits:

The above mentioned patches will be included in the upcoming v0.16.0 release.

Let me know what you think, whether I am missing something, or whether you disagree with this process proposal. I am happy to take care of the v0.16.0 release in the upcoming weeks.

Version bump will follow in an upcoming PR.

simonpasquier and others added 18 commits May 5, 2018 11:14
* cli: extract client bindings of the v1 API from amtool

This is a continuation of [1] but the code is kept in the alertmanage
repository rather than having it in client_golang.

[1] prometheus/client_golang#333

Co-Authored-By: Fabian Reinartz <fab.reinartz@gmail.com>
Co-Authored-By: Tristan Colgate <tcolgate@gmail.com>
Co-Authored-By: Corin Lawson <corin@responsight.com>
Co-Authored-By: stuart nelson <stuartnelson3@gmail.com>

* cli: fix httpSilenceAPI.Set() method

* vendor: remove github.com/prometheus/client_golang/api/alertmanager

* cli: don't use the model.Alert type
* Update silence add/update flags

- Change --expires/-e to --duration/-d
- Change --expires-on to --end
- Add --start

* update subcommand returns ID of new silence

The silences printed before were accurate, except
they had the old ID. Now the new ID is returned.

* Duration is added to silence.StartsAt

When a user supplies a duration to update a
silence, it is applied to silence.StartsAt after
any potential changes to the silence's start time.
When the aggregation group receives an alert that is past the initial
group_wait value, it should reset its timer only if the timer has ever
expired. Otherwise it means that the flush is already in-progress.
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
* cli: move commands to cli/cmd

* cli: use StatusAPI interface for config command

* cli: use SilenceAPI interface for silence commands

* cli: use AlertAPI for alert command

* cli: move back commands to cli package

And move API client code to its own package.

* cli: remove unused structs
Within alertmanager, expire is the term used,
since silences still "exist" but aren't in effect.

Signed-off-by: Stuart Nelson <stuartnelson3@gmail.com>
* Move amtool to modular structure

Signed-off-by: Stuart Nelson <stuartnelson3@gmail.com>

* Move toplevel setup back into root.go

Signed-off-by: Stuart Nelson <stuartnelson3@gmail.com>

* Remove confusing alert struct name overwriting

A local variable within the alert subcommand was
using the name of the struct within that file.

Signed-off-by: Stuart Nelson <stuartnelson3@gmail.com>

* change local var name shadowing struct name

Signed-off-by: Stuart Nelson <stuartnelson3@gmail.com>
* inhibit: update inhibition cache when alerts resolve

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

* inhibit: remove unnecessary fmt.Sprintf

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

* inhibit: add unit tests

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

* inhibit: use NopLogger in tests

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

* Update old alert with result of merge with new

On ingest, alerts with matching fingerprints are
merged if the new alert's start and end times
overlap with the old alert's.

The merge creates a new alert, which is then
updated in the internal alert store.

The original alert is not updated (because merge
creates a copy), so it is never marked as resolved
in the inhibitor's reference to it.

The code within the inhibitor relies on skipping
over resolved alerts, but because the old alert is
never updated it is never marked as resolved. Thus
it continues to inhibit other alerts until it is
cleaned up by the internal GC.

This commit updates the struct of the old alert
with the result of the merge with the new alert.

An alternative would be to always update the
inhibitor's internal cache of alerts regardless of
an alert's resolve status.

Signed-off-by: stuart nelson <stuartnelson3@gmail.com>

* Update inhibitor cache even if alert is resolved

This seems like a better choice than the previous
commit. I think it is more sane to have the
inhibitor update its own cache, rather than having
one of its pointers updated externally.

Signed-off-by: stuart nelson <stuartnelson3@gmail.com>
* Use default values to store values from config
* fix typo and reserved keywork
* move to long help texts
* add one more unit test for resolver
* update comments

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
The unit test was making a request to the public Wechat endpoint which
caused flaky results.

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
This change upgrades the build configuration to CircleCI 2.0.

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
…#1360)

Signed-off-by: stuart nelson <stuartnelson3@gmail.com>
This was referenced May 5, 2018
@mxinden
Copy link
Member Author

mxinden commented May 5, 2018

As some of these commits pre-date the project wide Developer-Certificate-Of-Origin introduction, not all commits are signed off, hence the failed DCO-bot.

How do you want to proceed here?

//CC @stuartnelson3 @brian-brazil @juliusv

@stuartnelson3
Copy link
Contributor

I'm not sure what the implications are. I'm guessing we can just merge any commits that pre-date the adoption of the DCO, but since I'm not familiar with it I defer to @brian-brazil and @juliusv

@brian-brazil
Copy link
Contributor

We can just go ahead here, there's no new code.

@mxinden
Copy link
Member Author

mxinden commented May 7, 2018

Thanks @stuartnelson3 @brian-brazil.

I will merge once Prometheus operator e2e test suit succeeded: prometheus-operator/prometheus-operator#1308

Should give us at least a pretty good K8s test coverage.

@mxinden
Copy link
Member Author

mxinden commented May 9, 2018

@stuartnelson3 Prometheus operator test suit succeeded. Shall we move on with this PR, or wait for #1341?

@mxinden mxinden mentioned this pull request Jun 5, 2018
4 tasks
@mxinden
Copy link
Member Author

mxinden commented Jun 8, 2018

Closing here in favour of #1410.

@mxinden mxinden closed this Jun 8, 2018
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

5 participants