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

[MPDX-8446] Automate translation download and extraction #1209

Merged
merged 5 commits into from
Nov 26, 2024

Conversation

canac
Copy link
Contributor

@canac canac commented Nov 21, 2024

Description

  • On every commit to main run yarn extract before uploading labels to OneSky. That way translation can begin immediately on any new labels instead of needing to wait for public/locales/en/translation.json to be updated. (The extracted labels are not committed and are discarded when the action terminates)
  • During deployment to Amplify, run yarn onesky:download to deploy the app with the latest labels from OneSky. I tested that if that command fails for any reason, the deployment will still continue successfully.
    • I added the ONESKY_* environment variables to Amplify.
  • Add a new workflow that runs daily to create a PR with any new labels from yarn extract or yarn onesky:download. Here is an example from my fork of the PR that will be created: [no-Jira] Update translations canac/mpdx-react#5. If the PR hasn't merged, the action will rebase and force-push any new labels to the branch.

MPDX-8446

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

@canac canac added the Preview Environment Add this label to create an Amplify Preview label Nov 21, 2024
@canac canac requested a review from dr-bizz November 21, 2024 19:08
@canac canac self-assigned this Nov 21, 2024
Copy link
Contributor

github-actions bot commented Nov 21, 2024

Bundle sizes [mpdx-react]

Compared against 34edc48

No significant changes found

Copy link
Contributor

Preview branch generated at https://8446-automate-translations.d3dytjb8adxkk5.amplifyapp.com

@wrandall22
Copy link
Contributor

I tested that if that command fails for any reason, the deployment will still continue successfully.

Have we tested what happens when we get a timeout from OneSky?

@canac
Copy link
Contributor Author

canac commented Nov 21, 2024

Have we tested what happens when we get a timeout from OneSky?

@wrandall22 Not specifically. I tested the case where the environment variables aren't provided and it logs a 401 message and terminates with a successful error code. Are you afraid of it hanging forever if OneSky doesn't respond?

@wrandall22
Copy link
Contributor

Right, my concern would be it hanging forever or making the deployment timeout.

@canac
Copy link
Contributor Author

canac commented Nov 21, 2024

Right, my concern would be it hanging forever or making the deployment timeout.

@wrandall22 OK, I set it up to timeout after one minute. It takes 10 seconds to run locally on my machine, so that should be plenty of time. I tested in a branch that timeout 1s sleep 5 || echo "Timed out" prints the message and doesn't stop the deployment.

amplify.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

This is looking great! I left some comments

env:
# Disable git hooks because we are only modifying translation files
HUSKY: 0
with:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to add reviewers to this?

Copy link
Contributor

Choose a reason for hiding this comment

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

What are your thoughts about adding delete-branch: true to delete thre branch after each merge and so the branch is only around if changes are to be made.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are your thoughts about adding delete-branch: true to delete thre branch after each merge and so the branch is only around if changes are to be made.

We have this repo configured to automatically delete branches from merged PRs, so that shouldn't be an issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to add reviewers to this?

Adding team reviewers requires a Personal Access Token with repo scope or making a GitHub App with those permissions (https://github.com/peter-evans/create-pull-request/tree/v7/#action-inputs). If we use a PAT, we have to deal with rotating it when it expires and having to get someone else to create one if the person who created it leaves our GitHub org or the web-engineering-js team. It's also less secure to use a long-lived PAT instead of the short-lived automatic GITHUB_TOKEN. Making a GitHub App for this also doesn't feel worth it.

If the goal is to be alerted when this PR gets created, I think we can mention @CruGlobal/web-engineering-js in the PR body, and we'll all get a notification.

.github/workflows/onesky.yml Show resolved Hide resolved
@canac canac requested a review from dr-bizz November 25, 2024 20:38
Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

Thank you for making this! Lets test it out and review after some time

@canac canac force-pushed the 8446-automate-translations branch from 7547442 to 687b86d Compare November 26, 2024 16:10
@canac canac enabled auto-merge November 26, 2024 16:11
@canac
Copy link
Contributor Author

canac commented Nov 26, 2024

Lets test it out and review after some time

@dr-bizz Yes, it would be good to revisit this maybe in the new year to see if we would like to make any adjustments.

@canac canac merged commit b29df9f into main Nov 26, 2024
17 checks passed
@canac canac deleted the 8446-automate-translations branch November 26, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Preview Environment Add this label to create an Amplify Preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants