Skip to content

re-enable deployment test#17751

Merged
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
deads2k:rebase-02-deployment-check
Dec 15, 2017
Merged

re-enable deployment test#17751
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
deads2k:rebase-02-deployment-check

Conversation

@deads2k
Copy link
Contributor

@deads2k deads2k commented Dec 13, 2017

fixes #17750.

Adding debugging for a start.

@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 13, 2017
This was referenced Dec 13, 2017
Merged
Closed
@deads2k
Copy link
Contributor Author

deads2k commented Dec 13, 2017

Weird. I must not have pushed my fix to this. I'll do it tomorrow.

@deads2k deads2k force-pushed the rebase-02-deployment-check branch from 083f551 to 551f02f Compare December 14, 2017 12:56
@openshift-ci-robot
Copy link

openshift-ci-robot commented Dec 14, 2017

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

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

@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 Dec 14, 2017
@deads2k
Copy link
Contributor Author

deads2k commented Dec 14, 2017

/assign @tnozicka
/assign @soltysh

tnozicka
tnozicka reviewed Dec 14, 2017
View reviewed changes
pkg/oc/cli/cmd/rollout/cancel.go
if len(patches) == 0 {
allPatchesEmpty := true
for _, patch := range patches {
if len(patch.Patch) > 0 {
Copy link
Contributor

@tnozicka tnozicka Dec 14, 2017

Choose a reason for hiding this comment

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

can there be patch '{}'? otherwise lgtm except the debugs

Copy link
Contributor Author

@deads2k deads2k Dec 14, 2017

Choose a reason for hiding this comment

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

can there be patch '{}'? otherwise lgtm except the debugs

I don't think that produces the same error. I'm actually wondering if the server changed to reject empty patches...

Copy link
Contributor

@tnozicka tnozicka Dec 14, 2017

Choose a reason for hiding this comment

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

other places use constructs like this

if len(patch.Patch) > 0 && string(patch.Patch) != "{}"

https://github.com/openshift/origin/blob/master/pkg/oc/cli/cmd/set/triggers.go#L332-L335

Copy link
Contributor

@sttts sttts Dec 15, 2017

Choose a reason for hiding this comment

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

CreateTwoWayMergePatch looks sane in 1.8 and 1.9, both giving `{}´ for no changes.

@tnozicka
Copy link
Contributor

tnozicka commented Dec 14, 2017

/hold
just in case (as it contains unwanted debugs)

fix looks ok

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2017
@deads2k
Copy link
Contributor Author

deads2k commented Dec 14, 2017

#17769

/retest

@sttts
Copy link
Contributor

sttts commented Dec 14, 2017 •
edited
Loading

deads: I'm wondering if CalculatePatches used to provide a zero length list and now produces a list with an empty patch

Tested empty strategic merge patch with a local cluster via

curl -v -XPATCH -H "Content-Type: application/strategic-merge-patch+json" -H "User-Agent: kubectl/before (darwin/amd64) kubernetes/2a70dc8" -H "Accept: application/json" http://127.0.0.1:8080/api/v1/namespaces/default

With 1.8: {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"EOF","code":500}

With 1.9: {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"EOF","reason":"BadRequest","code":400}

@tnozicka
Copy link
Contributor

tnozicka commented Dec 14, 2017

flake #17787
/retest

@deads2k deads2k force-pushed the rebase-02-deployment-check branch from 551f02f to 56ac0bf Compare December 14, 2017 16:38
@deads2k deads2k added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 14, 2017
@deads2k
Copy link
Contributor Author

deads2k commented Dec 14, 2017

Tested with a local cluster via

I'm wondering CalculatePatches is now returning an empty string instead of {}. That would be a serialization problem for JSONMap.

@deads2k
Copy link
Contributor Author

deads2k commented Dec 14, 2017

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Dec 14, 2017

#17770

@deads2k
Copy link
Contributor Author

deads2k commented Dec 14, 2017

/retest

@openshift-merge-robot
Copy link
Contributor

openshift-merge-robot commented Dec 15, 2017

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 89bbf2a into openshift:master Dec 15, 2017
@deads2k deads2k deleted the rebase-02-deployment-check branch January 24, 2018 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jim-minter jim-minter Awaiting requested review from jim-minter

@knobunc knobunc Awaiting requested review from knobunc

2 more reviewers

@sttts sttts sttts left review comments

@tnozicka tnozicka tnozicka left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@soltysh soltysh

@tnozicka tnozicka

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/post-rebase 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

re-enable "should only deploy the last deployment"

6 participants

@deads2k @openshift-ci-robot @tnozicka @sttts @openshift-merge-robot @soltysh