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

Update yarn install step from dev guide #5067

Merged
merged 4 commits into from
Sep 22, 2023

Conversation

BSFishy
Copy link
Contributor

@BSFishy BSFishy commented Sep 19, 2023

Description

Update developer guide after #5065 was merged. I was concerned because everything I've seen says to install yarn through corepack, so I did some digging. The old dev guide said to install yarn using the latest version from yarn trunk (v3 something), which would break bootstrap and everything with that. However, OSD is built using yarn v1, which is incompatible with yarn v3. Essentially, there are significant differences between yarn v1 and v3, which make all of the guides in Dashboards break, since they were built for v1 instead of v3. This PR updates the guides to use corepack, which is the recommended way to install yarn, but to still install v1.

Issues Resolved

N/a

Screenshot

N/a

Testing the changes

Ran this on Node version 14, 16, and 18 to get yarn v1.22.18 installed and tested clean and bootstrap with that version.

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Signed-off-by: Matt Provost <provomat@amazon.com>
Signed-off-by: Matt Provost <provomat@amazon.com>
@joshuarrrr joshuarrrr added Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry v2.11.0 v1.3.14 backport 1.3 backport 2.x labels Sep 19, 2023

```bash
$ npm i -g yarn
$ corepack enable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, we had corepack prepare .... The prepare subcommand doesn't exist in newer versions of corepack: https://github.com/nodejs/corepack?tab=readme-ov-file#utility-commands

Instead, the enable subcommand should add all relevant package managers (including yarn).

joshuarrrr
joshuarrrr previously approved these changes Sep 19, 2023
manasvinibs
manasvinibs previously approved these changes Sep 19, 2023
Copy link
Member

@manasvinibs manasvinibs left a comment

Choose a reason for hiding this comment

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

Nice!

@dtaivpp
Copy link
Contributor

dtaivpp commented Sep 19, 2023

Thanks!

@BSFishy
Copy link
Contributor Author

BSFishy commented Sep 20, 2023

Additionally, we could modify it to be something like this:

$ # Update corepack to the latest version
$ npm i -g corepack

$ # Install the correct version of yarn
$ corepack install

And have something like this in our package.json:

{
  "packageManager": "yarn@1.22.19"
}

That will potentially make this even better, by ensuring everyone has the same version of yarn installed.

(reference: https://github.com/nodejs/corepack?tab=readme-ov-file#when-authoring-packages)

@joshuarrrr
Copy link
Member

Additionally, we could modify it to be something like this:

$ # Update corepack to the latest version
$ npm i -g corepack

$ # Install the correct version of yarn
$ corepack install

And have something like this in our package.json:

{
  "packageManager": "yarn@1.22.19"
}

That will potentially make this even better, by ensuring everyone has the same version of yarn installed.

(reference: https://github.com/nodejs/corepack?tab=readme-ov-file#when-authoring-packages)

I'd definitely prefer that direction even more. Anything we can do to standardize the process and eliminate potential ambiguity would be great!

Signed-off-by: Matt Provost <provomat@amazon.com>
@BSFishy BSFishy dismissed stale reviews from manasvinibs and joshuarrrr via 727ebb7 September 20, 2023 18:51
Signed-off-by: Matt Provost <provomat@amazon.com>
@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #5067 (73509dd) into main (b9b2b79) will not change coverage.
Report is 3 commits behind head on main.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #5067   +/-   ##
=======================================
  Coverage   66.50%   66.50%           
=======================================
  Files        3403     3403           
  Lines       65026    65026           
  Branches    10401    10401           
=======================================
  Hits        43243    43243           
  Misses      19214    19214           
  Partials     2569     2569           
Flag Coverage Δ
Linux_1 34.82% <ø> (ø)
Linux_2 55.31% <ø> (ø)
Linux_3 44.60% <ø> (ø)
Linux_4 34.89% <ø> (ø)
Windows_1 34.83% <ø> (ø)
Windows_2 55.27% <ø> (ø)
Windows_3 44.61% <ø> (ø)
Windows_4 34.89% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@joshuarrrr joshuarrrr merged commit 46d7c2d into opensearch-project:main Sep 22, 2023
54 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 1.3 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-1.3 1.3
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-1.3
# Create a new branch
git switch --create backport/backport-5067-to-1.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 46d7c2d8c1f3af737697ee984d31ea1cccf85494
# Push it to GitHub
git push --set-upstream origin backport/backport-5067-to-1.3
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-1.3

Then, create a pull request where the base branch is 1.3 and the compare/head branch is backport/backport-5067-to-1.3.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-5067-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 46d7c2d8c1f3af737697ee984d31ea1cccf85494
# Push it to GitHub
git push --set-upstream origin backport/backport-5067-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-5067-to-2.x.

@BSFishy BSFishy deleted the docs/developer-yarn branch September 23, 2023 01:33
joshuarrrr pushed a commit to joshuarrrr/OpenSearch-Dashboards that referenced this pull request Oct 3, 2023
* Update yarn install step from dev guide

Signed-off-by: Matt Provost <provomat@amazon.com>

* Update the corepack command

Signed-off-by: Matt Provost <provomat@amazon.com>

* Update to use corepack install

Signed-off-by: Matt Provost <provomat@amazon.com>

* Revise corepack url

Signed-off-by: Matt Provost <provomat@amazon.com>

---------

Signed-off-by: Matt Provost <provomat@amazon.com>
(cherry picked from commit 46d7c2d)
joshuarrrr pushed a commit to joshuarrrr/OpenSearch-Dashboards that referenced this pull request Oct 3, 2023
* Update yarn install step from dev guide

Signed-off-by: Matt Provost <provomat@amazon.com>

* Update the corepack command

Signed-off-by: Matt Provost <provomat@amazon.com>

* Update to use corepack install

Signed-off-by: Matt Provost <provomat@amazon.com>

* Revise corepack url

Signed-off-by: Matt Provost <provomat@amazon.com>

---------

Signed-off-by: Matt Provost <provomat@amazon.com>
(cherry picked from commit 46d7c2d)
joshuarrrr added a commit that referenced this pull request Oct 3, 2023
* Update yarn install step from dev guide

Signed-off-by: Matt Provost <provomat@amazon.com>

* Update the corepack command

Signed-off-by: Matt Provost <provomat@amazon.com>

* Update to use corepack install

Signed-off-by: Matt Provost <provomat@amazon.com>

* Revise corepack url

Signed-off-by: Matt Provost <provomat@amazon.com>

---------

Signed-off-by: Matt Provost <provomat@amazon.com>
(cherry picked from commit 46d7c2d)

Co-authored-by: Matt Provost <provomat@amazon.com>
Co-authored-by: Sean Neumann <1413295+seanneumann@users.noreply.github.com>
ananzh pushed a commit that referenced this pull request Oct 3, 2023
* Update yarn install step from dev guide
* Update the corepack command
* Update to use corepack install
* Revise corepack url

---------

Signed-off-by: Matt Provost <provomat@amazon.com>
(cherry picked from commit 46d7c2d)

Co-authored-by: Matt Provost <provomat@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.3 backport 2.x Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry v1.3.14 v2.11.0 valued-contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants