Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add Option for Volume Service Broker name #1266

Merged
merged 3 commits into from
Oct 17, 2024

Conversation

winkingturtle-vmw
Copy link
Contributor

@winkingturtle-vmw winkingturtle-vmw commented Oct 7, 2024

What is this change about?

Add Option for Volume Service Broker name

Please provide contextual information.

If VolumeService Service broker is configured with a different broker-name, cats will fail. With this change, it'll be required to provide a volume_service_broker_name only when running volume-services tests

What version of cf-deployment have you run this cf-acceptance-test change against?

main

Please check all that apply for this PR:

  • introduces a new test --- Are you sure everyone should be running this test?
  • changes an existing test
  • requires an update to a CATs integration-config

Did you update the README as appropriate for this change?

  • YES
  • N/A

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?

N/A

What is the level of urgency for publishing this change?

  • Urgent - unblocks current or future work (our pipeline's are currently broken)
  • Slightly Less than Urgent

Tag your pair, your PM, and/or team!

borkername could be different depending on how the broker was deployed. We deploy
volume-services with multiple brokers and name is required to be able to run this test.
winkingturtle-vmw added a commit to cloudfoundry/wg-app-platform-runtime-ci that referenced this pull request Oct 7, 2024
@ctlong
Copy link
Member

ctlong commented Oct 8, 2024

What did the -b flag for cf enable-service-access default to before? I think ideally this change should set that as the default to avoid a breaking change

@winkingturtle-vmw
Copy link
Contributor Author

It didn't have any default. It picked the one available service-broker. I didn't provide a default, because I wasn't sure what most people would've tested for, but I am happy to add a default value of nfsbroker if that's what the default pipelines are meant to test. Let me know.

Copy link
Member

@davewalter davewalter left a comment

Choose a reason for hiding this comment

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

@winkingturtle-vmw Is the intention for this to be a required configuration when running the volume services tests? If so, it would be good to add a check to the validateVolumeServices helper function to ensure correct configuration. If not, then I think the changes to volume_services/volume_services.go need to be adjusted to not provide the -b flag to enable-service-access and create-service when the service broker name is empty.

@winkingturtle-vmw
Copy link
Contributor Author

I've updated this PR to have a backward compatible behavior. Currently, we are running this PR through our pipeline to see if we missed anything.

@davewalter
Copy link
Member

I've updated this PR to have a backward compatible behavior. Currently, we are running this PR through our pipeline to see if we missed anything.

Thanks @winkingturtle-vmw. Please let us know if your CI went green and we can go ahead and merge this.

@winkingturtle-vmw
Copy link
Contributor Author

Above changes has gone green in our pipeline.

@ctlong ctlong merged commit 29a9479 into cloudfoundry:develop Oct 17, 2024
3 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants