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

Update start_db_helper.sh #3187

Closed
wants to merge 1 commit into from
Closed

Update start_db_helper.sh #3187

wants to merge 1 commit into from

Conversation

strehle
Copy link
Member

@strehle strehle commented Dec 11, 2024

No description provided.

Copy link
Contributor

@Kehrlann Kehrlann left a comment

Choose a reason for hiding this comment

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

I think there are two issues with this:

A. If you run ./gradlew test, several gradle modules may be tested in parallel, and so there WILL be multiple workers.
B. As the comment above states, "We make extra dbs because a gradle worker ID can exceed the max number of workers." In the test I just ran, I got workers 7 and 8

This change is only acceptable if:
1. You run without a gradle daemon, e.g. in a brand new container, in CI, with --no-daemon, or after ./gradlew --stop
2. AND you run tests for a single project, e.g. ./gradlew cloudfoundry-identity-server:test

So this would hinder our ability to run tests locally because of we want to use gradle daemons locally (so, not 1.); and would very likely break CI whenever we merge #3186 because in CI we just run "test" on all projects (not 2) .

I think this will require changes in how the tests run in the first place ; either make projects depend on each other, or changes to how databases are provisioned.

Edit:

It is actually already failing in CI, probably because of A / not 2.

I will think about it over the week-end, but I think the correct move is to have to have three databases:

  1. uaa (which is there by default) for connecting a local UAA
  2. cloudfoundry-identity-server-test for tests
  3. cloudfoundry-identity-server-uaa for tests

@strehle happy to take care of this if you want.

@strehle
Copy link
Member Author

strehle commented Dec 15, 2024

I think there are two issues with this:

A. If you run ./gradlew test, several gradle modules may be tested in parallel, and so there WILL be multiple workers. B. As the comment above states, "We make extra dbs because a gradle worker ID can exceed the max number of workers." In the test I just ran, I got workers 7 and 8

This change is only acceptable if: 1. You run without a gradle daemon, e.g. in a brand new container, in CI, with --no-daemon, or after ./gradlew --stop 2. AND you run tests for a single project, e.g. ./gradlew cloudfoundry-identity-server:test

So this would hinder our ability to run tests locally because of we want to use gradle daemons locally (so, not 1.); and would very likely break CI whenever we merge #3186 because in CI we just run "test" on all projects (not 2) .

I think this will require changes in how the tests run in the first place ; either make projects depend on each other, or changes to how databases are provisioned.

Edit:

It is actually already failing in CI, probably because of A / not 2.

I will think about it over the week-end, but I think the correct move is to have to have three databases:

  1. uaa (which is there by default) for connecting a local UAA
  2. cloudfoundry-identity-server-test for tests
  3. cloudfoundry-identity-server-uaa for tests

@strehle happy to take care of this if you want.

@Kehrlann Thanks for your effort. It was not my intension to do a change here. Did it to see if multiple DBs are really needed

@strehle strehle closed this Dec 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants