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

Add a CAPI version check for HTTP/2 to certain integration tests #2195

Conversation

ctlong
Copy link
Member

@ctlong ctlong commented Aug 20, 2021

Does this PR modify CLI v6 or v7?

v8

Description of the Change

Adds a new CC version check to an integration test that (probably?) depends on certain CAPI. Paves the way for future HTTP/2 integration tests to be written in CLI that won't break pipelines.

Also adds one new integration test for if the route destination doesn't already exist and the DestinationProtocol flag is set.

Why Is This PR Valuable?

  • (Maybe) fix breaking pipelines
  • No pressure to upgrade environments that CLI uses for integration tests to enable HTTP/2
  • Easier testing of HTTP/2-specific tests, just point the integration tests at a CAPI of proper version and it'll work!

Why Should This Be In Core?

Fixes current core functionality.

Applicable Issues

cloudfoundry/routing-release#200

How Urgent Is The Change?

Somewhat urgent depending on if the last HTTP/2 PR is breaking pipelines.

@ctlong
Copy link
Member Author

ctlong commented Aug 20, 2021

From https://github.com/cloudfoundry/cli/blob/master/.github/CONTRIBUTING.md#api-versioning:

The feature has an appropriate version check in the command layer to prevent use of that feature if the targeted API is below the minimum version. Note: commands should FAIL prior to execution when minimum version is not met for specified functionality.

Should the CC version check be used to determine if users even see the option to use the beta flag @JenGoldstrich?

@JenGoldstrich
Copy link
Contributor

I think this should be fine so long as CAPI will accept http2 as a destination protocol as of that CAPI version, I was under the impression we'd need to change a CAPI property on all of our environments but if all we need is new enough CAPI this should work great.

@ctlong
Copy link
Member Author

ctlong commented Aug 20, 2021

There is a feature flag in the form of a bosh property, but it's in GoRouter, not CAPI. Since the CAPI HTTP/2 code is theoretically idempotent with the GoRouter and Diego HTTP/2 code, a CAPI version check should suffice.

I'm making some assumptions with choosing 3.104.0. The last CAPI release had CC V3 version 3.103.0 and the next CAPI release should have the HTTP/2 code.

(big thanks to seth for pointing out this was an option)

@ctlong ctlong marked this pull request as ready for review August 20, 2021 01:26
* Setting the DestinationProtocol flags could cause problems if CAPI V3
is not >= 3.104.0
* Adds one new integration test for if the route destination doesn't
already exist and the DestinationProtocol flag is set
@ctlong ctlong force-pushed the fix/destination-protocol-integration-tests branch from 211a4f4 to 2b38239 Compare August 20, 2021 17:57
Copy link
Contributor

@jdgonzaleza jdgonzaleza left a comment

Choose a reason for hiding this comment

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

LGTM

@jdgonzaleza jdgonzaleza merged commit ef091af into cloudfoundry:master Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants