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.

Sign up for GitHub

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

Jump to bottom

RFC: Monorepo #336

Merged
merged 13 commits into from
Feb 20, 2023
Merged

RFC: Monorepo #336

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b49d7fc
Write the monorepo RFC
dhruvkb Nov 24, 2022
3b8caba
Fix typo in repository name
dhruvkb Dec 1, 2022
f2122d8
Update incorrect info about reorganised files
dhruvkb Dec 1, 2022
d619394
Add required reviewers as suggested by @zackkrida
dhruvkb Dec 2, 2022
6ef0168
Expand the benefits of monorepo
dhruvkb Dec 7, 2022
f2a9d81
Update implementation to include the `WordPress/openverse` repo
dhruvkb Dec 7, 2022
205ca41
Update RFC with more details, specifically about deployment
dhruvkb Dec 15, 2022
9a1449d
Merge branch 'main' of https://github.com/WordPress/openverse into rf…
dhruvkb Dec 15, 2022
53fe443
Format file as per Prettier
dhruvkb Dec 15, 2022
5763d16
Update the instruction about copying
dhruvkb Dec 16, 2022
f88693a
Add the migration notice for the repos
dhruvkb Dec 16, 2022
91e056b
Fix bad grammar
dhruvkb Dec 16, 2022
3b429c3
Fix linter violations
dhruvkb Dec 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 63 additions & 9 deletions rfcs/20221124-monorepo.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,14 @@

# RFC: Monorepo

**Status:** 🚧 WIP, comments are welcome nonetheless

## Reviewers

- [ ] @zackkrida
- [ ] @sarayourfriend

## Rationale

For a comprehensive discussion about the pros, the cons and the counterpoints to each see [discussion](https://github.com/WordPress/openverse/issues/192). This is not the purpose of this RFC.

This RFC summarily lists the benefits and then, with the twin assumptions of a monorepo being ultimately beneficial and the decision to migrate being finalised in the above discussion, proceeds to go into the implementation details.
For a comprehensive discussion about the pros, the cons and the counterpoints to each see [discussion](https://github.com/WordPress/openverse/issues/192). Some of the more nuanced points are listed below, biased towards the overall benefits of a monorepo to justify the RFC. This RFC also proceeds to go into the implementation details hoping that the benefits are cumulatively enough of an improvement to convince everyone to migrate.

### Benefits of monorepo

Expand Down Expand Up @@ -79,13 +75,17 @@ First we will merge the API and the frontend repos into `WordPress/openverse`. T

1. The merge of two JavaScript codebases provides fertile ground for testing `pnpm` workspaces.

- It also allows us to merge the browser extension later and split the design system/component library stuff into a separate package.

1. The API is already organised by stack folders so the `frontend/` directory will fit right in with the others like `api/` and `ingestion_server/`. Similarly the scripts repo is nicely organised in folders, reducing conflicts.

1. The API and frontend share identical tooling for Git hooks, linting and formatting. We will fight our tools less and encounter minimal friction.

- The frontend's approach for `pre-commit` inspired the RFC for expaning this type of usage to the API as well!
- The frontend's approach for `pre-commit` expanded this type of usage to the API as well!

- We're expanding the use of double-quoted strings to JavaScript to further unify our style guides.

1. The entire system can be integration tested during releases. The real API, populated with test data, can even replace the Talkback server as long as we can turn off all network calls and enable 100% reliable output.
1. The entire system can be integration tested during releases. The real API, populated with test data, can replace the Talkback server as long as we disable network calls and make output deterministic.
Copy link
Contributor

Choose a reason for hiding this comment

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

I love this! 🚀

Copy link
Contributor

Choose a reason for hiding this comment

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

We would have to make sure that the API response types never differ from the sample data, though. For example, if we say that the license URL is always present, and it's present in the test data, but absent in the real API responses, that would break the app even though the tests pass.

Copy link
Member Author

Choose a reason for hiding this comment

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

Next step would be integrating the catalog so the supplier of the data would also be a part of the tests 👍.


The `WordPress/openverse` repo will absorb the `WordPress/openverse-api` and `WordPress/openverse-frontend` repos. The `WordPress/openverse-catalog` will also be merged, _later_.

Expand All @@ -101,6 +101,16 @@ The first step will be to release the API and frontend, call a code freeze on bo

This can prove difficult given how productive our team is, so we will need to channel this productivity towards the catalog in the meantime. I can foresee the end-to-end migration taking one week (ideal scenario) to becoming workable again, and another week (for us to iron out any gaps in the docs and references).

##### Timeline breakdown

- Day 1: Merging the repos and resolving conflicts, restoring broken workflows except deploys
- Day 2: Restoring deployment workflows including staging auto-deploy
- Day 3: Transfer of issues from individual repos to monorepo
- Day 4: Documentation fixes
- Day 5: Housekeeping

The second week is planned as a buffer in case any of these tasks ends up taking more time than a day, if something breaks or if someone falls ill etc. The ideal scenario is that we're completely back next week, the worst one takes two.

Note that in the transition period nothing will break. The old repos will continue to exists as they are, till we ensure everything works and then we archive the current split repos.

### Step 1: Merge with histories
Expand Down Expand Up @@ -232,10 +242,32 @@ With this done, we can archive the API and frontend repo. An optional notice may

#### Combine linting

All lint steps can be combined in `.pre-commit-config.yaml`. This also simplifies the CI jobs can now be merged.
All lint steps can be combined in `.pre-commit-config.yaml`. This also implies the CI jobs can now be merged.

1. Remove pre-commit scripts from `frontend/package.json` and the `install-pre-commit.sh` script.

1. Remove `lint` job from CI, there are plenty of those.

#### `pnpm` workspace

1. Move `frontend/.pnpmfile.cjs` outside to the root directory, update the reference to `frontend/package.json`.

1. Remove `frontend/.npmrc` created earlier because `pnpm` will automatically use the one in the root of the workspace.

1. Create `pnpm-workspaces.yaml` file, see https://github.com/dhruvkb/monoverse/blob/main/.pre-commit-config.yaml.

1. Update the `package.json` files, see the following:

- https://github.com/dhruvkb/monoverse/blob/main/package.json
- https://github.com/dhruvkb/monoverse/blob/main/frontend/package.json
- https://github.com/dhruvkb/monoverse/blob/main/automations/js/package.json

1. `pnpm i` in the monorepo root.

1. Update the recipe `pnpm` in `frontend/justfile` to include `--filter "openverse-frontend"`.

1. `git commit -m "Setup workspaces"`

### Step 3. Restore workflows

#### New actions
Expand All @@ -248,7 +280,29 @@ To clean up the workflows we will define three new actions. The code for all thr

#### Update workflows

With this done, the development on the API and frontend can continue inside their subdirectories. The development of both parts will be independent. At least until we reach [long-term consolidation](#step-5-long-term-consolidation).
Workflows with major changes:

- `ci_cd.yml` from the API will absorb `ci.yml` from the frontend
Copy link
Collaborator

@sarayourfriend sarayourfriend Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What approach will we take to avoid running API tests when only frontend code is changed and vice-versa? Combining the CI workflows will preclude us from using the workflow paths filter. Did you have a different solution in mind?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can use the path-filter action to do that.

- `lint.yml` will be deleted

Updates:

- `migration_safety_warning.yml`
- `generate_pot.yml`

With this done, the development on the API and frontend can continue inside their subdirectories. The development of both parts will be independent, at least until we reach [long-term consolidation](#step-5-long-term-consolidation).

#### Deployment

##### Staging

The soon-to-be-ECS-based API and ECS-based frontend will continue to deploy to staging via the CI + CD pipeline, with deployment starting as soon as all CI checks have passed. They will use similar code as the frontend auto-deploy for staging used currently.

These will be separate jobs with specific [path-based filters](https://github.com/dorny/paths-filter).
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the benefit of using this paths-filter action rather than the built-in GitHub workflow paths settings for workflow triggers? Similar to my other question: https://docs.github.com/en/actions/using-workflows/triggering-a-workflow#using-filters-to-target-specific-paths-for-pull-request-or-push-events

Copy link
Member Author

@dhruvkb dhruvkb Dec 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This actions allows us to filter jobs instead of applying the filter to the entire workflow. This allows us to have a complete workflow where the jobs can be made to run based on the outcome of other jobs with needs. For example we can run the jobs to deploy the docs to GH Pages and Docker images to GHCR only when the CI jobs have passed.

If these jobs were in separate workflows, I don't think it's possible to run a workflow's jobs if jobs in a different workflow have passed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is possible to have cross-workflow dependencies, actually: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_run

I only have two concerns with using the actions:

  1. Extremely repetitive and annoying code to maintain in the workflows (need to guard each frontend/api job with the same repeated step at the top of the job). Note that we could use YAML anchors to fix this slightly (still more repetition here compared to paths) but actionlint does not currently support them. We'd either need to contribute an upstream PR or stop using actionlint, the latter of which I think is unwise given how helpful it is at catching syntax errors locally.
  2. It still actually uses a runner and worker time to complete whereas needs bypasses it entirely, therefore not eating up any of our quota. Considering the sheer number of jobs we're going to end up having in a single repository by the end of this migration, cutting down on the number of jobs that are run is prudent in this respect. This is one reason why we added all the concurrency configuration to our workflows, for example.

If you think either of these are not a concern, then that is fine. The first should be documented as an expectation and accepted trade off. I think the second would bite us later on, but I can accept that I may be being too cautious.

One more thing that crossed my mind as I was writing this is whether our concurrency settings will need to change for jobs. My hunch is no, but it's worth looking into to confirm as they can be quite nasty to think through.

Copy link
Member Author

Choose a reason for hiding this comment

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

workflow_run workflows depend on the "OR" combination of multiple workflows. This is different from needs which depends the "AND" combination of multiple jobs.

The annoying to maintain code might be true, but it can be overcome by abstracting the paths-filter job into a separate action with some inputs (similar to the setup-env action), thus limiting the annoyance to a single file.

Point 2 could be an issue as it will spin up a job and cancel it if the criterion fails, an alternative to that could be to have a job that only matches the condition of the paths. Let's say jobs called "is_api", "is_frontend" etc. The other jobs can needs on them so that they're skipped if their code paths are untouched. I think it will be a bit of a hassle to manage the pass/fail/skip cascades though so it's something that needs a bit of a dry-run first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The other jobs can needs on them so that they're skipped if their code paths are untouched. I think it will be a bit of a hassle to manage the pass/fail/skip cascades though so it's something that needs a bit of a dry-run first.

I hadn't considered that, using a single (or handful of) job(s) that determine(s) which part of the project is being changed. It could actually be more flexible as the jobs could be is-frontend as you suggested, but also, if we know of specific cross-app dependencies that are fragile, we could have jobs for those as well (is-<whatever>-endpoint, etc). Then needs on other jobs actually sets up excellent self-documenting workflows and does not cause all the actions to run even if they're not needed. We'll have "failed" workflows, I guess, for when one of the project detection jobs doesn't match. I wonder how we would enforce "required" actions for different parts of the application? Can a job be required but skipped due to not matching the "needs"? I don't think so, in which case the secondary callable workflow you mentioned that takes a list of the relevant project parts for the job, while still tedious, at least allows some level of abstraction. Then we can still have required jobs that just automatically pass if they aren't relevant to the part of the code being changed.


##### Production

For production, we will not be able to use GitHub Releases and will have to use a manually-triggered workflow to build the assets and tag them. The tag can be set via a workflow input (simple) or can be calculated based on the update scope of major, minor or patch (not as simple).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another way to simplify this is for us to switch to SHA based tags for release deployment. Rather than creating a new build, just use the build tagged with the HEAD SHA.

Copy link
Member Author

Choose a reason for hiding this comment

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

The slight issue with SHA tags would be that it's harder to identify versioning with them. Given two hashes, it's not possible to say which is the newer version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, good point!


### Step 3. Housekeeping and DX cleanup

Expand Down