*: bump actix/actix-web to 0.8.3/1.0.2 #123

steveej · 2019-06-17T20:52:39Z

TODO

migrate to new API
cleanup unwraps
fix tests
investigate why the response body from the policy-engine is empty in case of an error
add a test case verifying the response body content

cincinnati/src/plugins/interface.rs

lucab · 2019-06-18T09:21:09Z

policy-engine/src/graph.rs

This will result in a client-controlled panic if len < 2, right?

Indeed! where len < 2 means the query argument doesn't contain a =. In that case we should probably give a meaningful error to the client.

Since the processing of the query string isn't in the future chain it's not straight forward to generate an error for the user here. The current behavior in master is that such values are filled in with the empty string, so I'm opting for retaining that.

@lucab let me know if you agree, then I'll squash the fixup commit.

Ack. I agree it makes sense to either use an empty string, or filter-out valueless keys (assuming we'll never need a valueless key).

graph-builder/src/graph.rs

policy-engine/src/graph.rs

graph-builder/src/main.rs

steveej · 2019-06-18T19:43:35Z

On local tests I noticed that the errors aren't reported as JSON strings to the client from the policy-engine. Instead the body is empty. I added some more tests to the graph::index method which seem fine. I suspect there has also been change in how the errors are converted into responses. I'll investigate that tomorrow.

steveej · 2019-06-24T14:52:36Z

@lucab this one's ready for the next round. PTAL

commons/src/errors.rs

graph-builder/src/graph.rs

graph-builder/src/status.rs

policy-engine/src/graph.rs

policy-engine/src/main.rs

policy-engine/src/openapi.rs

lucab · 2019-06-25T08:17:33Z

@steveej thanks for bumping it! I did another pass and only spot a few minor things, so I think this is mostly ready to land. There are some manifest+lockfile conflicts to adjust after the prometheus bump.

steveej · 2019-06-25T15:33:25Z

@lucab Thanks for the review! All comments are addressed and the rebase is complete. The latter required me to adapt the tests you had introduced in #125, I hope I got them right. PTAL

lucab · 2019-06-25T17:43:24Z

/lgtm

openshift-ci-robot · 2019-06-25T17:43:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lucab, steveeJ

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:

~~OWNERS~~ [lucab,steveeJ]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

steveej requested review from lucab and shiywang June 17, 2019 20:52

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 17, 2019

steveej commented Jun 17, 2019

View reviewed changes

cincinnati/src/plugins/interface.rs Outdated Show resolved Hide resolved

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 17, 2019

steveej force-pushed the pr/bump-actix-web branch from f261276 to 5bf8a92 Compare June 18, 2019 09:17

lucab reviewed Jun 18, 2019

View reviewed changes

steveej force-pushed the pr/bump-actix-web branch 4 times, most recently from d3b4724 to 73d53d2 Compare June 18, 2019 11:38

steveej changed the title ~~[WIP] *: bump actix/actix-web to 0.8.3/1.0.2~~ *: bump actix/actix-web to 0.8.3/1.0.2 Jun 18, 2019

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 Jun 18, 2019

steveej requested a review from lucab June 18, 2019 11:38

steveej force-pushed the pr/bump-actix-web branch 3 times, most recently from 2b1c32a to 9b2b453 Compare June 18, 2019 11:41

lucab reviewed Jun 18, 2019

View reviewed changes

graph-builder/src/graph.rs Outdated Show resolved Hide resolved

policy-engine/src/graph.rs Outdated Show resolved Hide resolved

steveej force-pushed the pr/bump-actix-web branch from 9b2b453 to 9bdd7ca Compare June 18, 2019 15:24

lucab reviewed Jun 18, 2019

View reviewed changes

graph-builder/src/main.rs Outdated Show resolved Hide resolved

steveej force-pushed the pr/bump-actix-web branch from 2984cd2 to d0448ef Compare June 18, 2019 17:44

steveej changed the title *: bump actix/actix-web to 0.8.3/1.0.2 [WIP] *: bump actix/actix-web to 0.8.3/1.0.2 Jun 18, 2019

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 18, 2019

steveej force-pushed the pr/bump-actix-web branch from d0448ef to 8b51001 Compare June 18, 2019 19:41

steveej force-pushed the pr/bump-actix-web branch 2 times, most recently from 193b47d to e1c1e1d Compare June 24, 2019 11:32

steveej changed the title ~~[WIP] *: bump actix/actix-web to 0.8.3/1.0.2~~ *: bump actix/actix-web to 0.8.3/1.0.2 Jun 24, 2019

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 Jun 24, 2019

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2019

lucab reviewed Jun 25, 2019

View reviewed changes

steveej force-pushed the pr/bump-actix-web branch from e1c1e1d to 0959f2d Compare June 25, 2019 14:13

steveej added 6 commits June 25, 2019 16:30

*: bump actix/actix-web to 0.8.3/1.0.2

61271e9

cincinnati: regenerate plugin interface after protobuf bump

cc76c0c

*: migrate to new API of actix/actix-web

56570e6

*: minor code cleanup

10aa815

Cargo/policy-engine: add mockito dev-dependency

d8c98ed

common/errors: make some read-only methods public

b8d2224

steveej force-pushed the pr/bump-actix-web branch from 0959f2d to 7ba092e Compare June 25, 2019 15:31

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2019

policy-engine/graph: add unit and integration tests

1961401

steveej force-pushed the pr/bump-actix-web branch from 7ba092e to 1961401 Compare June 25, 2019 16:17

openshift-ci-robot assigned lucab Jun 25, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2019

openshift-merge-robot merged commit 6e9e0f2 into openshift:master Jun 25, 2019

steveej deleted the pr/bump-actix-web branch June 25, 2019 17:48

*: bump actix/actix-web to 0.8.3/1.0.2 #123

*: bump actix/actix-web to 0.8.3/1.0.2 #123

Uh oh!

Conversation

steveej commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lucab Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

steveej Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

steveej Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveej Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

lucab Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steveej commented Jun 18, 2019

Uh oh!

steveej commented Jun 24, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lucab commented Jun 25, 2019

Uh oh!

steveej commented Jun 25, 2019

Uh oh!

lucab commented Jun 25, 2019

Uh oh!

openshift-ci-robot commented Jun 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

steveej commented Jun 17, 2019 •

edited

Loading

steveej Jun 18, 2019 •

edited

Loading

lucab Jun 18, 2019 •

edited

Loading