-
Notifications
You must be signed in to change notification settings - Fork 16
docs: Update dev-standards and local deploy docs #3349
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
Conversation
cf03c5f to
3bb6fd9
Compare
SanjayVas
left a comment
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.
@SanjayVas reviewed 2 files and made 1 comment.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @laureanobrs).
src/main/k8s/local/README.md line 298 at r1 (raw file):
## Running the Correctness Test *Note*: currently the assertions in this test fail when running it in a local environment because the [test expects a large population](https://github.com/world-federation-of-advertisers/cross-media-measurement/blob/v0.5.29/src/test/kotlin/org/wfanet/measurement/integration/k8s/SyntheticGeneratorCorrectnessTest.kt#L99) and the local EDP simulators are [configured to use a small population](https://github.com/world-federation-of-advertisers/cross-media-measurement/blob/v0.5.29/src/main/k8s/local/edp_simulators.cue#L44). For the test to pass, it needs to be switched to population small.
nit: rephrase this to include clear instructions on what code changes to make to get the test to pass
Additional context would be that the local EDP simulators use a small population as single machines tend not to have enough memory to handle large populations. If they have enough RAM and are willing to wait longer for the test to run, they can technically swap the simulators to use the large pop instead of swapping the test to use the small one. We don't need to include instructions for that though :)
SanjayVas
left a comment
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.
@SanjayVas reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @laureanobrs).
53eca41 to
1953fb3
Compare
laureanobrs
left a comment
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.
@laureanobrs resolved 1 discussion.
Reviewable status: 2 of 3 files reviewed, all discussions resolved (waiting on @SanjayVas).
1953fb3 to
62ae61a
Compare
laureanobrs
left a comment
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.
@laureanobrs reviewed 3 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @laureanobrs).
No description provided.