-
Notifications
You must be signed in to change notification settings - Fork 117
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
feat(tooling): convert-repo-to-yarn-workspace #1443
feat(tooling): convert-repo-to-yarn-workspace #1443
Conversation
c5626c0
to
56a4aa4
Compare
The reason why github actions is failling is because the base branch is configured for npm. Once we merge this PR all the subsequent PRs should be configured for yarn and will pass. Since there is no yarnrc or yarn.lock file in base branch github actions is using npm for this PR. Example PR against create-workspace branch -https://github.com/Shreyas281299/react-widgets/actions/runs/11476200784/job/31935688832 |
0c7bb4b
to
c8d1b0e
Compare
c8d1b0e
to
af1ea91
Compare
REACT_WEBEX_VERSION: "", | ||
FEDERATION: "", | ||
IDBROKER_BASE_URL: "", | ||
U2C_SERVICE_URL: "", | ||
WEBEX_CLIENT_ID: "", | ||
WDM_SERVICE_URL: "", | ||
WEBEX_CONVERSATION_DEFAULT_CLUSTER: "", | ||
WEBEX_TEST_USERS_CONVERSATION_SERVICE_URL: "", | ||
}), |
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.
After moving to yarn workspaces, there is a different way in which env variables are passed. This change does nothing but provides a fallback to these empty string just in case these variables are not available. Im still trying to figure out how yarn workspaces work with env varibales.
A possible speculation is that previously all the commands were run from root folder and the .env folder was also present in the root folder. But now when we run yarn build for each workspace the root folder becomes the workspace and there is no .env available there.
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.
Mostly looks good. Just a few comments
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. Nit - just modify the yarn version from 4.5.0 to 4.5.1
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Mostly good. However,
- We do not this file to be checked-in:
.yarn/install-state.gz
as per this documentation. Let us remove it before merging the PR. Quoting the same here:.yarn/install-state.gz is an optimization file that you shouldn't ever have to commit. It simply stores the exact state of your project so that the next commands can boot without having to resolve your workspaces all over again.
- Please change the PR title
- The CI seems to be failing. Please check that one too
Removed the yarn/install-state file For the pipelines failure- |
This takeaway was incorrect. The actually issue was that github was not checking out the correct commit. |
Completes -SPARK-559074
My suggestion on how to review this PR - close the packages folder, it only has file name changes. After that its a small PR
This PR is the second PR in the series of many PRs to create react-widgets a yarn workspace
First PR #1438
This PR completes the final 4tasks in the list below
Tasks required to make react-widgets a yarn workspace
Checks
Testing
Issues found
All these errors are persistent in master and node-upgrade branch also