Skip to content

Conversation

@soltysh
Copy link
Contributor

@soltysh soltysh commented Apr 5, 2019

No description provided.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 5, 2019
@soltysh soltysh force-pushed the fix_legacy_podautoscaler branch from 9e2f4ed to c510a9b Compare April 5, 2019 09:57
@soltysh
Copy link
Contributor Author

soltysh commented Apr 5, 2019

/test unit

@soltysh
Copy link
Contributor Author

soltysh commented Apr 5, 2019

/test unit

return true, &v1.Event{}, nil
}
obj := action.(core.CreateAction).GetObject().(*v1.Event)
obj, ok := action.(core.CreateAction).GetObject().(*v1.Event)
Copy link
Member

Choose a reason for hiding this comment

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

So the client-go code says the same event (without defining what the same means) is patched when it's has been already recorded [1]. The fix would be then to allow to accept even Patch events. Hoping the Reason will stay the same. Or, just ignore any patch event.

[1] https://github.com/kubernetes/client-go/blob/master/tools/record/event.go#L214-L216

@soltysh soltysh force-pushed the fix_legacy_podautoscaler branch from c510a9b to 56740a7 Compare April 5, 2019 11:21
@soltysh soltysh changed the title WIP: Fix flaky legacy pod autoscaler test UPSTREAM: 76189: Fix flaky legacy pod autoscaler test Apr 5, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2019
@soltysh
Copy link
Contributor Author

soltysh commented Apr 5, 2019

That unit flake is a separate one.
/retest

@soltysh
Copy link
Contributor Author

soltysh commented Apr 5, 2019

/retest

2 similar comments
@soltysh
Copy link
Contributor Author

soltysh commented Apr 5, 2019

/retest

@soltysh
Copy link
Contributor Author

soltysh commented Apr 5, 2019

/retest

obj := action.(core.CreateAction).GetObject().(*v1.Event)
create, ok := action.(core.CreateAction)
if !ok {
return false, nil, nil
Copy link
Member

Choose a reason for hiding this comment

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

Per

// ReactionFunc is a function that returns an object or error for a given
// Action.  If "handled" is false, then the test client will ignore the
// results and continue to the next ReactionFunc. 

lgtm. Just ignoring every patch operation.

@ingvagabund
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2019
@derekwaynecarr
Copy link
Member

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, ingvagabund, soltysh

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

The pull request process is described here

Details 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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2019
@rphillips
Copy link
Contributor

/retest

@smarterclayton
Copy link
Contributor

Force merging to unblock the queue (all tests passed once)

@smarterclayton smarterclayton merged commit 777c79e into openshift:master Apr 5, 2019
@soltysh soltysh deleted the fix_legacy_podautoscaler branch April 5, 2019 19:23
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. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants