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

Create github release via CircleCI only for mozilla fork #349

Merged
merged 2 commits into from
Feb 17, 2022

Conversation

abhi-agg
Copy link
Contributor

@abhi-agg abhi-agg commented Feb 14, 2022

Fixes #335

The extension uses translator artifacts build in mozilla fork. Therefore, create github release via circleci only when running in mozilla fork.

Tested it here: https://app.circleci.com/pipelines/github/abhi-agg/bergamot-translator/96/workflows/10b2a8b5-32c0-4f36-abc1-1c379a1b876c

@abhi-agg abhi-agg force-pushed the circleci-ghrelease-fix branch 3 times, most recently from 5f0b425 to 69d498e Compare February 14, 2022 14:13
@abhi-agg abhi-agg marked this pull request as draft February 14, 2022 15:37
@abhi-agg abhi-agg force-pushed the circleci-ghrelease-fix branch 3 times, most recently from 9c27cf2 to 1a58371 Compare February 14, 2022 16:34
@abhi-agg abhi-agg marked this pull request as ready for review February 14, 2022 16:37
Copy link
Contributor

@jerinphilip jerinphilip left a comment

Choose a reason for hiding this comment

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

What problem pertaining to development does this mechanism solve? Is there anything preventing Mozilla from using the GitHub release mechanism here (I see a SHA256 missing, but that's easy to solve)? Also as a plus, the GitHub release here finishes in under 5 minutes, compared to CircleCI's 10m.

If I remember correctly, there were some initial build issues during some fake FS based big wasm builds that GitHub had issues with and the mechanism went to CircleCI + GCP? Having resolved that with separate model buffers, looks to me like firefox-translations is using GitHub released artefacts now:

https://github.com/mozilla/firefox-translations/blob/283a584d3d5ea975b880b1d96abbcd7fcb1977cb/extension/model/engineRegistry.js#L3

The fork relevant to this PR is https://github.com/mozilla/bergamot-translator, which doesn't appear to be updated since December. There's also the additional overhead of this change of a human having to manually press the fetch and merge button or replicate tags (#287 (comment)). WASM builds (wormhole and no wormhole) are already running via GitHub CI and if the publish_to_github part is only operational on Mozilla fork, what is the point of keeping this .yml here?

@abhi-agg
Copy link
Contributor Author

abhi-agg commented Feb 14, 2022

Earlier Github workflows had some actions that were not allowed to run under mozilla github org. That was the main reason to switch to using CircleCI.

I will sync the mozilla fork with the upstream and check if github workflows specific errors are gone there. After that github workflows can be updated to create the releases to be used for extension. However, there are other high priority tasks than refactoring the CI right now and therefore, we plan to stick with CircleCI for sometime.

andrenatal
andrenatal previously approved these changes Feb 15, 2022
Copy link
Contributor

@jerinphilip jerinphilip left a comment

Choose a reason for hiding this comment

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

Tested it here: app.circleci.com/pipelines/github/abhi-agg/bergamot-translator/96/workflows/10b2a8b5-32c0-4f36-abc1-1c379a1b876c

The test appears to show only environment variables. The bash commands under the if remain untested?

.circleci/config.yml Show resolved Hide resolved
.circleci/config.yml Show resolved Hide resolved
Copy link
Contributor

@jerinphilip jerinphilip left a comment

Choose a reason for hiding this comment

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

Tested it here: app.circleci.com/pipelines/github/abhi-agg/bergamot-translator/96/workflows/10b2a8b5-32c0-4f36-abc1-1c379a1b876c

The test appears to show only environment variables. The bash commands under the if remain untested?

Copy link
Contributor

@jerinphilip jerinphilip left a comment

Choose a reason for hiding this comment

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

Tested it here: app.circleci.com/pipelines/github/abhi-agg/bergamot-translator/96/workflows/10b2a8b5-32c0-4f36-abc1-1c379a1b876c

The test appears to show only environment variables. The bash commands under the if remain untested?

jerinphilip
jerinphilip previously approved these changes Feb 15, 2022
Copy link
Contributor

@jerinphilip jerinphilip left a comment

Choose a reason for hiding this comment

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

I try to evaluate changes from a technical standpoint. I believe I have highlighted above that the modus-operandi surrounding the positioning and maintenance of this CircleCI job is convoluted and unsustainable. We have already gone through a cron-job, a bot account and now this, all the while we can connect browsermt/bergamot-translator and mozilla/firefox-translations directly. Keeping mozilla/bergamot-translator in between creates unnecessary manual movement in place of automation.

Given that this does not affect the library, please go ahead with the merge if this speeds you up. Please excuse me from reviews on this file ahead.

 - The extension uses mozilla fork for translator artifacts
   -- Hence create github release via circleci only when
      running in mozilla fork
@abhi-agg abhi-agg dismissed stale reviews from jerinphilip and andrenatal via b37c16a February 17, 2022 16:20
@abhi-agg abhi-agg force-pushed the circleci-ghrelease-fix branch 2 times, most recently from d835970 to 80d7701 Compare February 17, 2022 16:45
@abhi-agg
Copy link
Contributor Author

We have already gone through a cron-job, a bot account and now this, all the while we can connect browsermt/bergamot-translator and mozilla/firefox-translations directly. Keeping mozilla/bergamot-translator in between creates unnecessary manual movement in place of automation.

This discussion is a waste of everyone's time. But to provide you some context, the decision of using mozilla/bergamot-translator for the extension dates long back when there existed no automated tests or release mechanism in this repo and older extension was still functional.

Btw, I synced mozilla fork and the latest github action fails there because of the use of
marvinpinto/action-automatic-releases@latest, jamesives/github-pages-deploy-action@4.1.3, and marvinpinto/action-automatic-releases@latest are not allowed to be used in mozilla/bergamot-translator and this is what I wanted to check.

Anyway, I will submit a PR that should add everything that might be required for anyone to use the wasm specific artifacts from releases created in this repository as well.

I am going ahead with merging this PR.

@abhi-agg abhi-agg merged commit 6ccd4c6 into browsermt:main Feb 17, 2022
@abhi-agg abhi-agg deleted the circleci-ghrelease-fix branch February 17, 2022 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CircleCI release mechanism is broken
3 participants