Skip to content
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

Remove platform version header check comment #661

Merged
merged 1 commit into from
Jan 22, 2018

Conversation

eriknelson
Copy link
Contributor

Describe what this PR does and why we need it:

PR does nothing but remove a very old header check that has been commented out. Background on this is when we inherited our original http server from the template broker, it was checking for the presence of the X-Broker-API-Version, which MUST be sent by the platform to brokers. The broker has no obligation with regard to this header; it's there in case brokers rely on features that may not be present in the platform they're currently talking to. IIRC, I created this issue because at the time, the catalog was not sending the header.

I'm removing the comment (and want to close the issue) because I don't see an immediate need for us to re-introduce the check, especially considering we're often a project that is validating proposed features to the spec that do not yet exist in OSB master. The issue should be reopened, or a new issue filed if we find a need to cooperate with platforms that are not the k8s service catalog (could forsee this as we start to talk about the BrokerSDK).

Which issue this PR fixes (This will close that issue when PR gets merged)
fixes #34

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 19, 2018
@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 19, 2018
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 3889924 on eriknelson:rm-handler-header-comment into ** on openshift:master**.

@coveralls
Copy link

coveralls commented Jan 19, 2018

Coverage Status

Changes Unknown when pulling 3889924 on eriknelson:rm-handler-header-comment into ** on openshift:master**.

Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

Failed LINT check

=================================
              Lint               
=================================
pkg/apb/types.go:318:2: struct field HttpProxy should be HTTPProxy
pkg/apb/types.go:319:2: struct field HttpsProxy should be HTTPSProxy
Found 2 lint suggestions; failing.
Makefile:27: recipe for target 'lint' failed
make: *** [lint] Error 1
The command "./scripts/travis.sh lint" exited with 2.

@jmrodri
Copy link
Contributor

jmrodri commented Jan 19, 2018

Rebase so it picks up the linter fixes

Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

👍

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 022c9f6 on eriknelson:rm-handler-header-comment into ** on openshift:master**.

@eriknelson
Copy link
Contributor Author

@jmrodri rebased

@rthallisey
Copy link
Contributor

Merging without a bug since it removes a comment.

@rthallisey rthallisey merged commit 77bf9f6 into openshift:master Jan 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Reintroduce Broker-API header restriction when k8s issue is settled
6 participants