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

Use Random Routes in Test Applications #123

Merged
merged 3 commits into from
Sep 13, 2024
Merged

Conversation

hoffmaen
Copy link
Contributor

@hoffmaen hoffmaen commented Sep 12, 2024

The sample-app-test workflow currently fails, because someone else deployed these sample apps in his/her own CF space on the same instance. Thus, the routes are takes and cannot be reassigned to apps in our CF space.

With this change, we prefix the routes with the Workflow Run ID, which should always give us unique routes.

Side note 1: cf push comes with the optional --no-route flag, unfortunately it's not working with mta deployments. That's why we have the yq step to remove all routes from the manifest before deploying it.

Side note 2: We cannot use random-route: true in the manifest: The GRPC tests require http2, and unfortunately the protocol for random routes defaults to http1 and cannot be overwritten (unless using cf update-destination). Hence, random-route is not an option, as it would break the scenario where a user deploys the test apps.

Copy link

cla-assistant bot commented Sep 12, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

cla-assistant bot commented Sep 12, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

grpcurl $ROUTE.${{ secrets.CF_DOMAIN }}:443 example.Example.Run
ROUTE=${{ github.run_id }}-java-grpc-test
cf8 map-route java-grpc-test ${{ secrets.CF_DOMAIN }} --hostname $ROUTE --app-protocol http2
grpcurl -proto http2/java-grpc/app/src/main/proto/example.proto $ROUTE.${{ secrets.CF_DOMAIN }}:443 example.Example.Run
Copy link
Collaborator

Choose a reason for hiding this comment

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

the only thing different is the path to the proto files. These files are all identical and just kept with the examples for completeness. There is no good reason to explicitly use different ones. In fact it would be better to use a single one to ensure that the examples all do the same thing and not something different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, the grpcurl command for the go-grpc-test app differs from the remaining ones. I'd like to keep the focus on fixing the route issue in this Pull-Requst, and do other improvements separately.

@@ -56,6 +56,8 @@ jobs:
cf8 --version
- name: Build Java apps
run: ./http2/gradlew clean build -p http2
- name: Remove routes from manifest
run: yq 'del(.applications[].routes)' -i "${GITHUB_WORKSPACE}/http2/apps-manifest.yml"
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of deleting, could we define a (default empty) prefix variable?

e.g.

application:
  route: "((prefix))some-app-name.((domain))"

and then define prefix: "" in the vars.yml, to override it when deploying?

@peanball peanball merged commit 5117c1d into main Sep 13, 2024
2 of 3 checks passed
@peanball peanball deleted the fix/use-random-routes branch September 13, 2024 10:12
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.

2 participants