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

Use verdaccio for the template e2e test #34577

Closed
wants to merge 3 commits into from

Conversation

fortmarek
Copy link
Contributor

@fortmarek fortmarek commented Sep 2, 2022

Summary

This PR adds verdaccio to release packages in the packages directory during the E2E test on CI.

The rationale behind this is the following:

  • Firstly, we wanted to push the monorepo RFC. We hit an issue when renaming the packages to follow the same convention caused by the e2e test using the template to fail. This is because the template installs packages from the live version of npm – and if you just rename a package in a given PR without releasing it, the package understandably can't be installed since it's not published, yet – as you can see here.
  • Secondly, the current e2e test on main does not actually test the latest code of the packages in the packages directory as it simply downloads the latest versions from npm. This creates a divide between what's tested and what users should expect when using nightlies or when a new minor is released.

Changelog

[Internal] [Changed] - Use verdaccio for template e2e test

Test Plan

test_js CI check should pass. Additionally, I have temporarily updated the PR renaming assets to assets-registry to include the verdaccio changes – the test_js passes there, additionally proving merging this PR will unblock us with the rename PRs.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Shopify Partner: Shopify Partner labels Sep 2, 2022
@fortmarek fortmarek force-pushed the verdaccio branch 4 times, most recently from d10087b to f078bb1 Compare September 2, 2022 10:48
@analysis-bot
Copy link

analysis-bot commented Sep 2, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 6529383
Branch: main

@analysis-bot
Copy link

analysis-bot commented Sep 2, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,698,673 +0
android hermes armeabi-v7a 7,102,261 +0
android hermes x86 8,001,419 +0
android hermes x86_64 7,973,988 +0
android jsc arm64-v8a 9,569,410 +0
android jsc armeabi-v7a 8,336,421 +0
android jsc x86 9,509,836 +0
android jsc x86_64 10,101,516 +0

Base commit: 6529383
Branch: main

@fortmarek fortmarek force-pushed the verdaccio branch 19 times, most recently from 83a5efe to 73c56c8 Compare September 6, 2022 20:47
@@ -70,6 +72,25 @@ try {

const REACT_NATIVE_PACKAGE = path.join(ROOT, 'react-native-*.tgz');

describe('Verdaccio');
const verdaccioProcess = child_process.spawn('npx', [
'verdaccio',
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see this added to dependencies, so I think we want to lock the version somehow, you can do it like this:

Suggested change
'verdaccio',
'verdaccio@5.15.3',

'yarn',
['start', '--max-workers 1'],
{
env: process.env,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the default, so you can delete these options afaik

@fortmarek fortmarek requested a review from oblador September 12, 2022 20:29
@fortmarek fortmarek changed the title Verdaccio Use verdaccio for the template e2e test Sep 12, 2022
keepAlive: true
maxSockets: 40
maxFreeSockets: 10
packages:

Choose a reason for hiding this comment

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

I'd recommend optimisations like facebook/docusaurus#7967 to avoid the registry proxy private packages to outside (publish might fails due network conditions) also help to keep isolated the packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is perfect, thank you so much 🤞

publish: $all
proxy: npmjs
logs:
- {type: file, path: verdaccio.log, format: pretty, level: debug}

Choose a reason for hiding this comment

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

Debug is really verbose, I'd recommend warn and also pretty is pretty expensive in terms of execution, if you don't need the coolers or fancy arrows, you might consider json instead to boost the registry.

@fortmarek fortmarek requested review from juanpicado and removed request for oblador September 14, 2022 13:21
@cortinico cortinico mentioned this pull request Sep 14, 2022
11 tasks
@cortinico
Copy link
Contributor

Thanks for doing this @fortmarek. I haven't had the time to review this yet, but I've posted an update on the Monorepo effort here: #34692 so we're aligned on the next steps

Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Great stuff 👍

scripts/run-ci-e2e-tests.js Show resolved Hide resolved
scripts/run-ci-e2e-tests.js Outdated Show resolved Hide resolved
.circleci/verdaccio.yml Outdated Show resolved Hide resolved
Comment on lines +35 to +38
'@*/*':
access: $all
publish: $authenticated
proxy: npmjs
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This prohibits unauthenticated users to published organization-scoped packages. I've seen this being done both in the Microsoft PR and the docusaurus one. @juanpicado, do you know the exact reasoning behind this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Great thanks for the clarification 👍

@fortmarek fortmarek force-pushed the verdaccio branch 4 times, most recently from ec56dd1 to e2f3ac0 Compare September 21, 2022 20:31
@fortmarek fortmarek force-pushed the verdaccio branch 2 times, most recently from d82ef32 to 9e2f6e6 Compare September 21, 2022 20:38
@fortmarek
Copy link
Contributor Author

Thanks for the review, the comments have been addressed 🤞

@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @fortmarek in 22940e4.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Sep 22, 2022
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This PR adds [verdaccio](https://github.com/verdaccio/verdaccio) to release packages in the `packages` directory during the E2E test on CI.

The rationale behind this is the following:
- Firstly, we wanted to push the [monorepo RFC](react-native-community/discussions-and-proposals#480). We hit an issue when renaming the packages to follow the same convention caused by the e2e test using the template to fail. This is because the template installs packages from the live version of npm – and if you just rename a package in a given PR without releasing it, the package understandably can't be installed since it's not published, yet – as you can see [here](https://app.circleci.com/pipelines/github/facebook/react-native/15286/workflows/149df51f-f59b-4eb3-b92c-20c513111f04/jobs/282135?invite=true#step-108-283).
- Secondly, the current e2e test on `main` does not actually test the latest code of the packages in the `packages` directory as it simply downloads the latest versions from npm. This creates a divide between what's tested and what users should expect when using nightlies or when a new minor is released.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[Internal] [Changed] - Use verdaccio for template e2e test

Pull Request resolved: facebook#34577

Test Plan: `test_js` CI check should pass. Additionally, I have temporarily updated the [PR](facebook#34572) renaming `assets` to `assets-registry` to include the verdaccio changes – the `test_js` passes there, additionally proving merging this PR will unblock us with the rename PRs.

Reviewed By: cipolleschi

Differential Revision: D39723048

Pulled By: cortinico

fbshipit-source-id: aeff3811967360740df3b3dbf1df50e506fb72d8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Shopify Partner: Shopify Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Tech: Monorepo For PRs that are related to the monorepo infra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants