-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[Tour Of Beam][backend] integration tests and GA workflow #23032
Conversation
Assigning reviewers. If you would like to opt out of this review, comment R: @Abacn for label build. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
lgtm |
R: @damccorm |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
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.
This generally looks good to me, left a few small comments
# This allows a subsequently queued workflow run to interrupt previous runs | ||
concurrency: | ||
group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}' | ||
cancel-in-progress: true |
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.
I wasn't familiar with this feature - neat!
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.
FYI, I added #23089 because I think we should be doing this more broadly - thanks for introducing it to the repo 😄
- uses: actions/checkout@v3 | ||
- uses: actions/setup-go@v3 | ||
with: | ||
go-version: '1.16' |
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.
Minor: Could we add a comment explaining we're pinning to 1.16 because Google Cloud Functions don't support higher versions (ideally here and in tour_of_beam_backend.yml)? That way we avoid a well meaning contributor (or dependabot) bumping the version.
learning/tour-of-beam/backend/integration_tests/function_test.go
Outdated
Show resolved
Hide resolved
@@ -24,7 +24,7 @@ FROM google/cloud-sdk:$GCLOUD_SDK_VERSION | |||
VOLUME /opt/data | |||
|
|||
# RUN mkdir -p /opt/data/WEB-INF | |||
COPY index.yaml /opt/data/WEB-INF/index.yaml | |||
# COPY index.yaml /opt/data/WEB-INF/index.yaml |
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.
Should we just remove this entirely? Why did we end up moving this?
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.
This was my initial idea of how to validate Datastore indexes: by copying it to the Datastore emulator container
But that actually doesn't work: emulator updates WEB-INF/index.yaml when it executes the query which requires a composite index.
So, in this PR we switch to comparing the generated index.yaml with our VCS version after the tests
Assuming integration tests covered every possible Datastore query, it produces an index that must work when deployed to Cloud
The line has to be removed, I overlooked that I only commented it
Co-authored-by: Danny McCormick <[email protected]>
Co-authored-by: Danny McCormick <[email protected]>
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.
LGTM - thanks!
* tests * WF * chdir * order * nit * fix * nit * concurrency * integration_go * tags * cache_go * nit * nit * rat * datadir * env * env * reduce fun * removing unicode character * Update learning/tour-of-beam/backend/integration_tests/client.go Co-authored-by: Danny McCormick <[email protected]> * Update learning/tour-of-beam/backend/integration_tests/function_test.go Co-authored-by: Danny McCormick <[email protected]> * review * nit Co-authored-by: oborysevych <[email protected]> Co-authored-by: Danny McCormick <[email protected]>
* tests * WF * chdir * order * nit * fix * nit * concurrency * integration_go * tags * cache_go * nit * nit * rat * datadir * env * env * reduce fun * removing unicode character * Update learning/tour-of-beam/backend/integration_tests/client.go Co-authored-by: Danny McCormick <[email protected]> * Update learning/tour-of-beam/backend/integration_tests/function_test.go Co-authored-by: Danny McCormick <[email protected]> * review * nit Co-authored-by: oborysevych <[email protected]> Co-authored-by: Danny McCormick <[email protected]>
* tests * WF * chdir * order * nit * fix * nit * concurrency * integration_go * tags * cache_go * nit * nit * rat * datadir * env * env * reduce fun * removing unicode character * Update learning/tour-of-beam/backend/integration_tests/client.go Co-authored-by: Danny McCormick <[email protected]> * Update learning/tour-of-beam/backend/integration_tests/function_test.go Co-authored-by: Danny McCormick <[email protected]> * review * nit Co-authored-by: oborysevych <[email protected]> Co-authored-by: Danny McCormick <[email protected]>
addresses #23005
Basic integration tests, GitHub action-wrapped, and a bit of a boilerplate to run it locally
Essentially an acceptance test.
Example workflow run:
https://github.com/akvelon/beam/runs/8192798498
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username
).addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.