Skip to content

track deployment scaleRefs for hpas#18150

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
juanvallejo:jvallejo/track-deployment-scaleRefs
Jan 19, 2018
Merged

track deployment scaleRefs for hpas#18150
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
juanvallejo:jvallejo/track-deployment-scaleRefs

Conversation

@juanvallejo
Copy link
Contributor

@juanvallejo juanvallejo commented Jan 17, 2018

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1534956

A ScalingEdgeKind was not being added between hpas and scaling refs of type extensions.Deployment

cc @deads2k @soltysh

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 17, 2018
@juanvallejo
Copy link
Contributor Author

juanvallejo commented Jan 17, 2018

/test extended_conformance_install
/test extended_conformance_gce

@juanvallejo
Copy link
Contributor Author

juanvallejo commented Jan 18, 2018

/retest

mfojtik
mfojtik reviewed Jan 18, 2018
View reviewed changes
pkg/api/kubegraph/edges.go
syntheticNode = kubegraph.FindOrCreateSyntheticReplicationControllerNode(g, &kapi.ReplicationController{ObjectMeta: syntheticMeta})
case appsapi.IsResourceOrLegacy("deploymentconfigs", r):
syntheticNode = appsgraph.FindOrCreateSyntheticDeploymentConfigNode(g, &appsapi.DeploymentConfig{ObjectMeta: syntheticMeta})
case r == kapisext.Resource("deployments"):
Copy link
Contributor

@mfojtik mfojtik Jan 18, 2018

Choose a reason for hiding this comment

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

@tnozicka do we need to handle apps.Resource("deployments) here as well?

Copy link
Contributor

@soltysh soltysh Jan 18, 2018

Choose a reason for hiding this comment

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

It's being handled in the previous case.

Copy link
Contributor

@mfojtik mfojtik Jan 18, 2018

Choose a reason for hiding this comment

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

i'm talking about kapps

Copy link
Contributor

@soltysh soltysh Jan 18, 2018

Choose a reason for hiding this comment

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

This is the internal representation. Depoyment's internal are extensions, still.

@mfojtik
Copy link
Contributor

mfojtik commented Jan 18, 2018

/approve no-issue

@tnozicka PTAL the question, feel free to LGTM if this is OK.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2018
soltysh
soltysh approved these changes Jan 18, 2018
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2018
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jan 18, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juanvallejo, mfojtik, soltysh

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

Details Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@soltysh
Copy link
Contributor

soltysh commented Jan 18, 2018

/retest

@mfojtik
Copy link
Contributor

mfojtik commented Jan 18, 2018

/retest (all known flakes)

@juanvallejo
Copy link
Contributor Author

juanvallejo commented Jan 18, 2018

/test extended_conformance_gce

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 18, 2018

/retest

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

@juanvallejo
Copy link
Contributor Author

juanvallejo commented Jan 19, 2018

/test extended_conformance_gce

@openshift-merge-robot
Copy link
Contributor

openshift-merge-robot commented Jan 19, 2018

Automatic merge from submit-queue (batch tested with PRs 18108, 18150, 18164, 18147, 18143).

@openshift-merge-robot openshift-merge-robot merged commit 9e66c98 into openshift:master Jan 19, 2018
@juanvallejo juanvallejo deleted the jvallejo/track-deployment-scaleRefs branch January 19, 2018 20:07
@juanvallejo
Copy link
Contributor Author

juanvallejo commented Jan 30, 2018

/cherrypick release-3.7
/cherrypick release-3.8

@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Jan 30, 2018

@juanvallejo: #18150 failed to apply on top of branch "release-3.7":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	pkg/api/kubegraph/edges.go
M	pkg/oc/cli/describe/projectstatus.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/oc/cli/describe/projectstatus.go
CONFLICT (content): Merge conflict in pkg/oc/cli/describe/projectstatus.go
Auto-merging pkg/api/kubegraph/edges.go
CONFLICT (content): Merge conflict in pkg/api/kubegraph/edges.go
Patch failed at 0001 track deployment scaleRefs for hpas
Details

In response to this:

/cherrypick release-3.7
/cherrypick release-3.8

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.

@juanvallejo juanvallejo mentioned this pull request Jan 30, 2018
Merged
openshift-merge-robot added a commit that referenced this pull request Jan 31, 2018
@openshift-merge-robot
Merge pull request #18344 from juanvallejo/jvallejo/pick-origin-pr-18150
47a7399 
Automatic merge from submit-queue.

Pick 18150: track deployment scaleRefs for hpas

Picks #18150
Backport part of fix for https://bugzilla.redhat.com/show_bug.cgi?id=1539876

Manual pick [due to merge conflicts](#18150 (comment)) with cherry-picking from original PR onto 3.8 / 3.7

cc @soltysh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@enj enj Awaiting requested review from enj

2 more reviewers

@mfojtik mfojtik mfojtik left review comments

@soltysh soltysh soltysh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@soltysh soltysh

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@juanvallejo @mfojtik @openshift-ci-robot @soltysh @openshift-bot @openshift-merge-robot @openshift-cherrypick-robot