-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
apps: replace legacy client with generated in pkg/deploy/cmd #16149
apps: replace legacy client with generated in pkg/deploy/cmd #16149
Conversation
type updateConfigFunc func(d *deployapi.DeploymentConfig) | ||
|
||
// updateConfigWithRetries will try to update a deployment config and ignore any update conflicts. | ||
func updateConfigWithRetries(dn appsinternal.DeploymentConfigsGetter, namespace, name string, applyUpdate updateConfigFunc) (*deployapi.DeploymentConfig, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably exists in some helper/util package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tnozicka do you know if we have this helperized already? i'm pretty sure we doing this in couple places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haven't seen such helper yet
} | ||
|
||
// DeploymentConfigHistoryViewer is an implementation of the kubectl HistoryViewer interface | ||
// for deployment configs. | ||
type DeploymentConfigHistoryViewer struct { | ||
rn kcoreclient.ReplicationControllersGetter | ||
dn client.DeploymentConfigsNamespacer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was just dead? weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah... maybe a leftover from some refact.
kc kclientset.Interface | ||
pollInterval, timeout time.Duration | ||
} | ||
|
||
type updateConfigFunc func(d *deployapi.DeploymentConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan. Why do we still have this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was just copy from the old client, seems like it is only used in one place, so right, we don't need this.
type updateConfigFunc func(d *deployapi.DeploymentConfig) | ||
|
||
// updateConfigWithRetries will try to update a deployment config and ignore any update conflicts. | ||
func updateConfigWithRetries(dn appsinternal.DeploymentConfigsGetter, namespace, name string, applyUpdate updateConfigFunc) (*deployapi.DeploymentConfig, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just shouldn't exist. If you want patch behavior, write a patch.
If this was pre-existing, open an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets fix that, low hanging fruit and I will forget about it: #16166
/lgtm |
824be43
to
ff5079f
Compare
/retest |
ff5079f
to
9ff8c20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -221,11 +222,15 @@ func (f *ring1Factory) LogsForObject(object, options runtime.Object, timeout tim | |||
|
|||
func (f *ring1Factory) Scaler(mapping *meta.RESTMapping) (kubectl.Scaler, error) { | |||
if deployapi.IsKindOrLegacy("DeploymentConfig", mapping.GroupVersionKind.GroupKind()) { | |||
oc, kc, err := f.clientAccessFactory.Clients() | |||
_, kc, err := f.clientAccessFactory.Clients() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this. Are we planning to change Clients()
to return openshift clientsets in the end? I do hope. Although probably it'll happen only when we actually remove pkg/client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this. Are we planning to change Clients() to return openshift clientsets in the end? I do hope. Although probably it'll happen only when we actually remove pkg/client
I think we will remove Clients
altogether and create separate helpers for each individual type of client.
There are no actual api changes here, just comments for the generation. Applying the api-approved label. |
/retest |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, mfojtik, soltysh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/unneeds-api-review |
/joke |
@mfojtik: I don't trust stairs. They're always up to something. In response to this:
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. |
Automatic merge from submit-queue |
Automatic merge from submit-queue apps: use patch when pausing dc on delete Ignore the first 2 commits, they are part of #16149 Don't try to emulate PATCH behavior with retrying updates on conflicts (of course patch can also conflict, but chances are low). Also we use PATCH for pausing in `oc rollout pause` already, so this just unify this behavior. Ref: #16149 (comment)
Ignore first 3 commits, they are part of #16100
@deads2k last bits in apps group