Fix deployment config scale subresource by liggitt · Pull Request #17517 · openshift/origin

liggitt · 2017-11-29T19:23:47Z

3.6 and 3.7 shipped with the /apis/apps.openshift.io/deploymentconfigs/scale subresource returning apps.openshift.io/v1 Scale objects

This is a non-standard Scale object that the HPA will never be able to make use of.

The intent was to continue send the same thing as /oapi/v1/deploymentconfigs/scale: extensions/v1beta1 Scale objects

This PR fixes the groupified API to send/receive the correct type.

It also updates the UpdateScale() client method to send extensions/v1beta1 Scale objects

liggitt · 2017-11-29T19:43:59Z

cc @deads2k @mfojtik @soltysh

mfojtik · 2017-11-29T20:21:59Z

code LGTM

liggitt · 2017-11-29T20:40:51Z

Will fix up generated bits

soltysh

lgtm to me as well

soltysh · 2017-11-29T20:59:31Z

pkg/apps/generated/clientset/typed/apps/v1/deploymentconfig_expansion.go

+
+// UpdateScale takes the top resource name and the representation of a scale and updates it. Returns the server's representation of the scale, and an error, if there is any.
+func (c *deploymentConfigs) UpdateScale(deploymentConfigName string, scale *v1beta1.Scale) (result *v1beta1.Scale, err error) {
+	// FIXME: make non-homogenous subresource GV client generation work


I'll sync with @sttts tomorrow and try to fix it in client generation. Unless you do it first.

liggitt · 2017-11-30T04:02:29Z

regenerated files, removed apps.openshift.io/v1 Scale from etcd test whitelist (no longer exists)

liggitt · 2017-11-30T04:33:40Z

/retest

soltysh · 2017-11-30T09:34:05Z

/retest

soltysh · 2017-11-30T11:54:00Z

Hmmm... template service broker should not be affected by this chanage
/retest

liggitt · 2017-11-30T16:37:56Z

removed unused import

liggitt · 2017-11-30T17:23:13Z

/hold cancel

deads2k · 2017-11-30T18:25:14Z

pkg/apps/generated/clientset/typed/apps/v1/deploymentconfig_expansion.go

+var scaleCodec = kapi.Codecs.LegacyCodec(v1beta1.SchemeGroupVersion)
+
+// UpdateScale takes the top resource name and the representation of a scale and updates it. Returns the server's representation of the scale, and an error, if there is any.
+func (c *deploymentConfigs) UpdateScale(deploymentConfigName string, scale *v1beta1.Scale) (result *v1beta1.Scale, err error) {


In 3.9, I'd rather have nothing than an untested, hand-written function in the external client. We can tell people to use the generic scale client.

For 3.7, I don't have a preference.

In 3.9, I'd rather have nothing than an untested, hand-written function in the external client.

It's exercised by CI... in what way is it untested?

deads2k · 2017-11-30T18:26:02Z

pkg/apps/generated/clientset/typed/apps/v1/deploymentconfig_expansion.go

+var scaleCodec = kapi.Codecs.LegacyCodec(v1beta1.SchemeGroupVersion)
+
+// UpdateScale takes the top resource name and the representation of a scale and updates it. Returns the server's representation of the scale, and an error, if there is any.
+func (c *deploymentConfigs) UpdateScale(deploymentConfigName string, scale *v1beta1.Scale) (result *v1beta1.Scale, err error) {


Actually, is the client function ever used in 3.7? If not, let's just leave it deleted.

yes, it's used

liggitt · 2017-11-30T18:47:45Z

/retest

liggitt · 2017-11-30T22:05:49Z

/retest

liggitt · 2017-12-01T04:35:57Z

/retest

deads2k · 2017-12-01T14:11:32Z

Thanks for the updates.

/lgtm

openshift-ci-robot · 2017-12-01T14:11:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt

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:

~~OWNERS~~ [deads2k,liggitt]

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

soltysh · 2017-12-01T15:18:20Z

/retest

openshift-bot · 2017-12-01T22:31:13Z

/retest

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

openshift-bot · 2017-12-02T20:39:34Z

/retest

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

openshift-bot · 2017-12-03T22:51:56Z

/retest

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

liggitt · 2017-12-04T14:39:15Z

/retest

0xmichalis · 2017-12-04T15:34:55Z

/retest

openshift-merge-robot · 2017-12-04T20:41:28Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-ci-robot · 2017-12-04T20:49:38Z

@liggitt: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_conformance_crio	`f698138`	link	`/test crio`
ci/openshift-jenkins/extended_conformance_install_update	`f698138`	link	`/test extended_conformance_install_update`

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.

Details

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 · 2017-12-04T22:10:44Z

Automatic merge from submit-queue.

openshift-ci-robot requested review from mfojtik and smarterclayton November 29, 2017 19:23

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 29, 2017

openshift-merge-robot added the needs-api-review label Nov 29, 2017

liggitt added this to the 3.7.0 milestone Nov 29, 2017

liggitt added the kind/bug Categorizes issue or PR as related to a bug. label Nov 29, 2017

soltysh approved these changes Nov 29, 2017

View reviewed changes

soltysh reviewed Nov 29, 2017

View reviewed changes

liggitt added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2017

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2017

deads2k reviewed Nov 30, 2017

View reviewed changes

liggitt added 2 commits November 30, 2017 16:13

Fix scale subresource type for apps.openshift.io/deploymentconfigs/scale

c5e3dc8

Fix deployment config UpdateScale

f698138

openshift-ci-robot assigned deads2k Dec 1, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2017

openshift-merge-robot merged commit 58a1f52 into openshift:release-3.7 Dec 4, 2017

liggitt mentioned this pull request Dec 12, 2017

Failed to scale a DC with oc #17729

Closed

liggitt mentioned this pull request Jan 15, 2018

the server doesn't have a resource type "dc" #17915

Closed

mshutt mentioned this pull request Mar 21, 2018

origin:v3.7.2 origin-master-controllers HPA works for deployments but not deploymentconfigs #19045

Closed

Conversation

liggitt commented Nov 29, 2017

Uh oh!

liggitt commented Nov 29, 2017

Uh oh!

mfojtik commented Nov 29, 2017

Uh oh!

liggitt commented Nov 29, 2017

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

soltysh Nov 29, 2017

Choose a reason for hiding this comment

Uh oh!

liggitt commented Nov 30, 2017

Uh oh!

liggitt commented Nov 30, 2017

Uh oh!

soltysh commented Nov 30, 2017

Uh oh!

soltysh commented Nov 30, 2017

Uh oh!

liggitt commented Nov 30, 2017

Uh oh!

liggitt commented Nov 30, 2017

Uh oh!

deads2k Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

liggitt Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

liggitt Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

liggitt commented Nov 30, 2017

Uh oh!

liggitt commented Nov 30, 2017

Uh oh!

liggitt commented Dec 1, 2017

Uh oh!

deads2k commented Dec 1, 2017

Uh oh!

openshift-ci-robot commented Dec 1, 2017

Uh oh!

soltysh commented Dec 1, 2017

Uh oh!

openshift-bot commented Dec 1, 2017

Uh oh!

openshift-bot commented Dec 2, 2017

Uh oh!

openshift-bot commented Dec 3, 2017

Uh oh!

liggitt commented Dec 4, 2017

Uh oh!

0xmichalis commented Dec 4, 2017

Uh oh!

openshift-merge-robot commented Dec 4, 2017

Uh oh!

openshift-ci-robot commented Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-merge-robot commented Dec 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

openshift-ci-robot commented Dec 4, 2017 •

edited

Loading