-
Notifications
You must be signed in to change notification settings - Fork 173
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
Update CF_INSTANCE_PORT and CF_INSTANCE_ADDR tests so they pass regar… #1203
Update CF_INSTANCE_PORT and CF_INSTANCE_ADDR tests so they pass regar… #1203
Conversation
…dless of unproxied port configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on cf-deployment v43.2.0, works. Also tested with PR cloudfoundry/cf-deployment#1195 merged, works!
I'll wait one more day to give the other ARD WG members a chance for review, then I'll merge the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Hmmmm are we okay with losing the present/not present check on the env vars? Currently, if I guess I would have expected that with the CF-D changes we would actually just want to switch |
Fair point. |
We can switch the default value, but then all the pipelines using older cd-d with the latest CATs will require manual intervention. If we don't, all the newer cf-d with any copy of CATs will require manual intervention. The reasoning for removing these checks was that they're very diego specific, and already tested in unit + integration tests by diego's pipelines. When digging back through the history of when they were added, it seemed like they were added to catch any specific problems that could trip up apps, but instead as satisfying a "this feature needs acceptance tests" checkbox. We discussed internally, and we were OK with increased risk by removing these tests as a trade off to making everyone reconfigure CATs to have it continue to work. But I'm happy to change this however y'all want. 😄 |
Are you submitting this PR against the develop branch?
yes
What is this change about?
Adjusts env var tests related to CF_INSTANCE_PORT and CF_INSTANCE_ADDR to pass regardless of how unproxied ports are configured for app containers.
Please provide contextual information.
Allows cloudfoundry/cf-deployment#1195 to no longer need changes to CATs configs
What version of cf-deployment have you run this cf-acceptance-test change against?
Latest.
Please check all that apply for this PR:
Did you update the README as appropriate for this change?
If you are introducing a new acceptance test, what is your rationale for including it CATs rather than your own acceptance test suite?
N/A
How many more (or fewer) seconds of runtime will this change introduce to CATs?
no change
What is the level of urgency for publishing this change?
Tag your pair, your PM, and/or team!
It's helpful to tag a few other folks on your team or your team alias in case we need to follow up later.