Skip to content

Conversation

@lucab
Copy link
Contributor

@lucab lucab commented Jan 16, 2019

This adds three mandatory parameters to cincinnati requests, as per
cincinnati-openshift specs.

Ref: https://github.com/openshift/cincinnati/blob/06c9ce64367b55f453295b2765a9aab61791dc76/docs/design/openshift.md#request

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 16, 2019
@lucab
Copy link
Contributor Author

lucab commented Jan 16, 2019

/hold
/cc @crawford

@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 Jan 16, 2019
@lucab
Copy link
Contributor Author

lucab commented Jan 16, 2019

/test e2e-aws

@crawford
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Jan 17, 2019
@smarterclayton
Copy link
Contributor

Can you guys add a Cincinnati integration test to pkg/start/start_integration_test.go (moved in #87) with a shim Cincinnati endpoint that verifies the behavior? We have functional tests in cvo_test but an extra higher level test will be good. Doesn't have to be here, just was looking at the bug and realized it would be a good candidate.

@lucab
Copy link
Contributor Author

lucab commented Jan 18, 2019

/test integration

@smarterclayton
Copy link
Contributor

/retest

@lucab
Copy link
Contributor Author

lucab commented Jan 24, 2019

/test integration

@steveej
Copy link
Contributor

steveej commented Jan 29, 2019

@smarterclayton is this blocked on the additional test you've asked for?

@lucab
Copy link
Contributor Author

lucab commented Jan 30, 2019

This was on hold due to the freeze being in place at the time, it should be fine now. My understanding is that this is not blocked on the suggested integration test, but I can add it if required (I only briefly skimmed through the existing one).

/hold cancel
/test integration

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 30, 2019
@smarterclayton
Copy link
Contributor

I would like a test showing the url being constructed in cvo_test.go

@smarterclayton
Copy link
Contributor

Or scenarios test or thenintegrstion test (the integration test is probably the best place because we can run a local server and report

@openshift-merge-robot
Copy link
Contributor

/retest

1 similar comment
@openshift-merge-robot
Copy link
Contributor

/retest

@lucab
Copy link
Contributor Author

lucab commented Feb 25, 2019

Superseded by #121.

@lucab lucab closed this Feb 25, 2019
@lucab lucab deleted the ups/cincinnati-query-params branch February 25, 2019 09:06
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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants