-
Notifications
You must be signed in to change notification settings - Fork 177
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
Add Google Translate documentation and simplify setup process #319
Conversation
…se64 google credentials.
We could actually enable comment translation in the e2e tests by storing the envvar as a secret. Assuming the secret is available in the PR's workflow run (i.e. if we're building a PR from a branch on the main repo instead of a fork), then we should be able to append the enabling envvars to the end of We'll also need to ensure that the tests against the comment translation features only run in that same scenario. diff --git a/.github/workflows/cypress-tests.yml b/.github/workflows/cypress-tests.yml
index 048493ee..86665ceb 100644
--- a/.github/workflows/cypress-tests.yml
+++ b/.github/workflows/cypress-tests.yml
@@ -39,16 +39,35 @@ jobs:
# and so newest version of client apps may not have been copied in.
docker-compose build file-server
+ - name: Enable comment translation
+ # If we're in a pull_request from a main repo branch, NOT a fork.
+ if: github.event.pull_request.head.repo.full_name == github.repository
+ run: |
+ echo GOOGLE_CREDENTIALS_BASE64=${{ secrets.GOOGLE_CREDENTIALS_BASE64 }} >> server/docker-dev.env
+ echo SHOULD_USE_TRANSLATION_API=true >> server/docker-dev.env
+
- name: Serve app via docker-compose
run: docker-compose up --detach
- - name: Run Cypress tests
+ - name: Run Cypress tests: default
uses: cypress-io/github-action@v1
env:
CYPRESS_BASE_URL: http://localhost
with:
working-directory: ./e2e
spec: cypress/integration/polis/**/*
+ config: ignoreTestFiles='**/polis/comment-translation/*'
+ browser: chrome
+
+ - name: Run Cypress tests: comment translation
+ # If we're in a pull_request from a main repo branch, NOT a fork.
+ if: github.event.pull_request.head.repo.full_name == github.repository
+ uses: cypress-io/github-action@v1
+ env:
+ CYPRESS_BASE_URL: http://localhost
+ with:
+ working-directory: ./e2e
+ spec: cypress/integration/polis/comment-translation/*
browser: chrome
# NOTE: screenshots will be generated only if E2E test failed |
I feel I should re-ticket the e2e tasks, since there are some slower coordination tasks related to getting secrets in the repo. So this is ready for review as-is. |
projectId: JSON.parse(fs.readFileSync(GOOGLE_CREDS_TEMP_FILENAME)).project_id, | ||
}); | ||
// Tell translation library where to find credentials, and write them to disk. | ||
process.env.GOOGLE_APPLICATION_CREDENTIALS = '.google_creds_temp' |
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 undocumented before, but I think it was required to set this envvar in the environment (or app, like we've done), in order to tell the Google client libs where to find the credentials. This allows the lib to load the credentials, since .google_creds_temp
is a nonstandard place to look.
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.
cc @metasoarous as to whether this is the right path going forward — is disk required here?
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.
to clarify, disk was always required :)
We were writing this file before:
https://github.com/pol-is/polisServer/pull/319/files#diff-ea75b530d74f94670cea053feb8b961bL231-L233
but we weren't hardcoding this envvar, that tells the Google client library where to find the credentials we'd written to disk -- presumably we were setting GOOGLE_APPLICATION_CREDENTIALS
as a heroku envvar (e.g., GOOGLE_APPLICATION_CREDENTIALS=.google_creds_temp
) or else the google library would've had no way to pick up the credentials that were written to this filepath by the old server code.
Made changes suggested by @colinmegill and ready for review by @metasoarous |
From Gitter, re
Merged my own suggestions that added this back in. |
One comment, but excellent to have this documented! Thanks @patcon |
Ok, got this working with e2e tests for Google Translation button feature. Here's a run with the Google credential available to the workflow: Here's the video artefact of the comment translation test run, for download: For now, I've created a service account and added $10 to pay for some Google Translate API use. Should last for a very long time. This can be merged without adding a Pls merge this, and then I can DM you the secret value on Gitter (if interested). Or alternatively leave it as-is, and we'll add credentials for Google later :) |
@colinmegill @metasoarous This is ready for review and merge -- bonus points if we do it synchronously on a call, so we can ensure the new e2e tests run after merging upstream :) |
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!
…mocracy#319) * Improved docs for comment translation setup. Changed to simplified base64 google credentials. * Initiated a CHANGELOG. * Added @ballPointPenguin props in docs for dockers. * Preserve GOOGLE_CREDS_STRINGIFIED envvar support. * Added dockerfile entry to prevent BUILDKIT error on GitHub Actions. See: npm/uid-number#3 (comment) * e2e: Ensure convo header appears before finishing createConvo command. * e2e: Added comment translation tests. * e2e: Test cleanup. Swapped English and French. * e2e: Ensure that workflow knows how to run tests with secrets. * Remove for testing on fork. * e2e: Fixed yaml typo. * e2e: Improved workflow logic for testing comment translation. * e2e: Fixed typo in bash syntax of workflow. * Added check to confirm status of containers on e2e test failures. * e2e:Simplify globbing for ignoreTestFiles. * e2e: Updated cypress from v4.7.0 to v4.9.0. * e2e: Renamed files for those generally requiring secrets. * e2e: Trying to avoid --spec arg to solve ignore issue. * e2e: Hopefully last fixup on ignored tests. * e2e: Prevent artefacts from prior runs getting trashed. * Brought back google creds management executables, removed in 04908fe. * Try another format for conditionals in workflow.
…mocracy#319) * Improved docs for comment translation setup. Changed to simplified base64 google credentials. * Initiated a CHANGELOG. * Added @ballPointPenguin props in docs for dockers. * Preserve GOOGLE_CREDS_STRINGIFIED envvar support. * Added dockerfile entry to prevent BUILDKIT error on GitHub Actions. See: npm/uid-number#3 (comment) * e2e: Ensure convo header appears before finishing createConvo command. * e2e: Added comment translation tests. * e2e: Test cleanup. Swapped English and French. * e2e: Ensure that workflow knows how to run tests with secrets. * Remove for testing on fork. * e2e: Fixed yaml typo. * e2e: Improved workflow logic for testing comment translation. * e2e: Fixed typo in bash syntax of workflow. * Added check to confirm status of containers on e2e test failures. * e2e:Simplify globbing for ignoreTestFiles. * e2e: Updated cypress from v4.7.0 to v4.9.0. * e2e: Renamed files for those generally requiring secrets. * e2e: Trying to avoid --spec arg to solve ignore issue. * e2e: Hopefully last fixup on ignored tests. * e2e: Prevent artefacts from prior runs getting trashed. * Brought back google creds management executables, removed in 04908fe. * Try another format for conditionals in workflow.
Re-ticketed from #263
To Do
GOOGLE_CREDENTIALS_BASE64
secret in forked repo and test -- @patconGOOGLE_CREDENTIALS_BASE64
secret in upstream repo (can merge without)chris@mathdem.org