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

Jump to bottom

Update node_exporter mixin and kubernetes mixin #456

Merged
merged 2 commits into from Sep 2, 2019
Merged

Update node_exporter mixin and kubernetes mixin #456

merged 2 commits into from Sep 2, 2019

Conversation

paulfantom
Copy link
Contributor

Merge #454 and #452 into one PR to bring in changes in one go.

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 28, 2019
@brancz
Copy link
Contributor

brancz commented Aug 28, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2019
@s-urbaniak
Copy link
Contributor

/lgtm

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@paulfantom
Copy link
Contributor Author

stopping retests

/hold

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm Indicates that a PR is ready to be merged. labels Aug 28, 2019
@brancz
Copy link
Contributor

brancz commented Aug 28, 2019

Just curious: why not update the Prometheus adapter config in kube prometheus? I would assume it’s similarly broken there no?

@s-urbaniak
Copy link
Contributor

@brancz yes, agreed, i just want to see this pass here.

@s-urbaniak s-urbaniak changed the title Update node_exporter mixin and kubernetes mixin WIP Update node_exporter mixin and kubernetes mixin Aug 28, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 28, 2019
@s-urbaniak
Copy link
Contributor

@brancz do you mind to have a look at 7324558#diff-f2a7750f46cf51bb668f12735f70777dR19 ? Is the query looking fine?

@paulfantom
Copy link
Contributor Author

This will close the following BZs:
https://bugzilla.redhat.com/show_bug.cgi?id=1733825
https://bugzilla.redhat.com/show_bug.cgi?id=1701856
https://bugzilla.redhat.com/show_bug.cgi?id=1733830

Additionally, we will need to revendor kube-prometheus, so it would be good to also revendor kubernetes-mixin. This way we will also close
https://bugzilla.redhat.com/show_bug.cgi?id=1745877 (cc: @metalmatze)

@paulfantom
Copy link
Contributor Author

/lgtm

@openshift-ci-robot
Copy link
Contributor

@paulfantom: you cannot LGTM your own PR.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@paulfantom paulfantom mentioned this pull request Aug 29, 2019
Closed
@brancz
Copy link
Contributor

brancz commented Aug 29, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2019
@s-urbaniak
Copy link
Contributor

leaving this on /hold until we merge openshift/console#2486 otherwise we'll regress on the console.

@s-urbaniak
Copy link
Contributor

Unfortunately I regressed one little thing in the resource overrides, submitting a fix to kube-prometheus.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2019
@s-urbaniak
Copy link
Contributor

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2019
@s-urbaniak
Copy link
Contributor

@brancz alright, this is good to go 👍

Synced with @spadgett that this will temporarily break node metrics in console which we already have WIP for fixing in openshift/console#2486

@lilic
Copy link
Contributor

lilic commented Aug 30, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2019
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

7 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2019
@s-urbaniak s-urbaniak mentioned this pull request Sep 2, 2019
Merged
@paulfantom
Copy link
Contributor Author

/retest

@s-urbaniak
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, LiliC, paulfantom, s-urbaniak

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [LiliC,brancz,paulfantom,s-urbaniak]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@s-urbaniak
Copy link
Contributor

/test e2e-aws-operator

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@paulfantom
Copy link
Contributor Author

/retest

1 similar comment
@paulfantom
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit df5f1b8 into openshift:master Sep 2, 2019
@paulfantom paulfantom deleted the mixins branch September 2, 2019 20:30
@s-urbaniak s-urbaniak mentioned this pull request Sep 3, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brancz brancz Awaiting requested review from brancz

@squat squat Awaiting requested review from squat

Assignees

@s-urbaniak s-urbaniak

@brancz brancz

@lilic lilic

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@paulfantom @brancz @s-urbaniak @openshift-bot @openshift-ci-robot @lilic @openshift-merge-robot