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

router - depricate -expose-metrics --metrics-image #19432

Merged
merged 1 commit into from
May 14, 2018

Conversation

pecameron
Copy link
Contributor

bug 1499026
https://bugzilla.redhat.com/show_bug.cgi?id=1499026
1499026 - Deprecate oc adm router --expose-metrics and --metrics-image

OCP 3.11

Signed-off-by: Phil Cameron pcameron@redhat.com

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 19, 2018
@pecameron
Copy link
Contributor Author

@knobunc @smarterclayton PTAL

bug 1499026
https://bugzilla.redhat.com/show_bug.cgi?id=1499026
1499026 - Deprecate oc adm router --expose-metrics and --metrics-image

origin issue: 15982
openshift#15982

OCP 3.11

Signed-off-by: Phil Cameron <pcameron@redhat.com>
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 19, 2018
@knobunc
Copy link
Contributor

knobunc commented Apr 19, 2018

@smarterclayton What is the deprecation policy around this? Can we just remove it? Or do we have to have a note in the docs and the command line flags for N releases and then we can remove the functionality after N + M releases?

@knobunc knobunc self-assigned this Apr 19, 2018
@pecameron
Copy link
Contributor Author

@knobunc @smarterclayton I think we added the deprecation note in 3.7.
Somebody else removed the code except for the command line options. The options before this PR were ignored, this PR removes them. Don't know when that happened.
docs pr 8803 removes the note.

@smarterclayton
Copy link
Contributor

smarterclayton commented Apr 20, 2018 via email

@dcbw
Copy link
Contributor

dcbw commented Apr 24, 2018

@smarterclayton how long is "a long time"? A couple origin releases?

@smarterclayton
Copy link
Contributor

3-4 releases

@pecameron
Copy link
Contributor Author

/retest

@pecameron
Copy link
Contributor Author

@knobunc PTAL

@knobunc
Copy link
Contributor

knobunc commented Apr 26, 2018

So... we can remove it in 3.11 if we deprecated in 3.7? (That would be 4 releases including 3.7)

@pecameron
Copy link
Contributor Author

@smarterclayton so is this ready to merge? Do we need to wait longer?

@smarterclayton
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 27, 2018
@pecameron
Copy link
Contributor Author

@smarterclayton @sdodson @knobunc extended_conformance_install tests expose-metrics and metrics-image. I have made changes in openshift-ansible PR 8177 to remove the tests.

@sdodson
Copy link
Member

sdodson commented May 1, 2018

/retest

@knobunc
Copy link
Contributor

knobunc commented May 1, 2018

/approve
/retest

@sdodson
Copy link
Member

sdodson commented May 1, 2018

/retest

@pecameron
Copy link
Contributor Author

/assign @smarterclayton

@pecameron
Copy link
Contributor Author

@knobunc looks like this is all set except another approval.

@sdodson
Copy link
Member

sdodson commented May 7, 2018

@juanvallejo bump for contrib approval

@juanvallejo
Copy link
Contributor

/lgtm

@pecameron
Copy link
Contributor Author

@knobunc @juanvallejo @sdodson @smarterclayton This has been hanging around for quite a while. Is there something we need to do to get this merged?

@sdodson
Copy link
Member

sdodson commented May 9, 2018

/approve

@sdodson sdodson added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 9, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: knobunc, pecameron, sdodson

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:

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

@sdodson
Copy link
Member

sdodson commented May 9, 2018

It needed to be approved by someone in contrib/completions/OWNERS but I'm taking juan's lgtm as implicit approved and i've added the label to the pr

@knobunc
Copy link
Contributor

knobunc commented May 9, 2018

/retest

@pecameron
Copy link
Contributor Author

@sdodson @knobunc It seems to be a timeout:
Readiness probe failed: Get https://10.128.0.12:8443/healthz: net/http: request canceled (Client.Timeout exceeded while awaiting headers)
Does this really relate to the changes in this PR?

@dcbw
Copy link
Contributor

dcbw commented May 14, 2018

/test extended_conformance_install

@openshift-ci-robot
Copy link

openshift-ci-robot commented May 14, 2018

@pecameron: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/unit 173e76b link /test unit

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 9643a7f into openshift:master May 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/routing lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants