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

opensearch-dashboards 1.2.0 #90403

Closed
wants to merge 2 commits into from
Closed

opensearch-dashboards 1.2.0 #90403

wants to merge 2 commits into from

Conversation

chenrui333
Copy link
Member

Created with brew bump-formula-pr.

@BrewTestBot BrewTestBot added bump-formula-pr PR was created using `brew bump-formula-pr` nodejs Node or npm use is a significant feature of the PR or issue labels Dec 4, 2021
@chenrui333
Copy link
Member Author

==> Testing opensearch-dashboards
unable to find usable node.js executable.
==> curl -s 127.0.0.1:57003

Error: opensearch-dashboards: failed
An exception occurred within a child process:
Minitest::Assertion: Expected: 0
Actual: 7

@chenrui333 chenrui333 added CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. test failure CI fails while running the test-do block labels Dec 4, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. To keep this pull request open, add a help wanted or in progress label.

@github-actions github-actions bot added the stale No recent activity label Dec 6, 2021
@chenrui333 chenrui333 force-pushed the bump-opensearch-dashboards-1.2.0 branch from 15e1b4b to 5382eec Compare December 6, 2021 19:45
@github-actions github-actions bot closed this Dec 7, 2021
@bayandin bayandin reopened this Dec 7, 2021
@bayandin bayandin force-pushed the bump-opensearch-dashboards-1.2.0 branch 2 times, most recently from 481acd9 to 6c67d67 Compare December 7, 2021 23:51
@github-actions github-actions bot closed this Dec 9, 2021
@bayandin bayandin reopened this Dec 9, 2021
@bayandin bayandin added python-3.10-migration and removed stale No recent activity test failure CI fails while running the test-do block labels Dec 9, 2021
carlocab
carlocab reviewed Dec 9, 2021
View reviewed changes
Formula/opensearch-dashboards.rb Outdated
Comment on lines 18 to 24
resource "yarn" do
url "https://registry.npmjs.org/yarn/-/yarn-1.22.17.tgz"
sha256 "267982c61119a055ba2b23d9cf90b02d3d16c202c03cb0c3a53b9633eae37249"
end
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the same as the yarn formula?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, wait. That yarn uses node. Maybe it'll just work? 😄

Copy link
Member

@bayandin bayandin Dec 9, 2021 •
edited

Choose a reason for hiding this comment

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

Ah, right, probably I should move up this comment from the bottom 🙂

# We can't depend on yarn formula since it brings the latest node
#  (which is getting picked up by some of the scripts and causes build failures)
# Repalce with `depends_on "yarn" => :build` when migraded to the latest node

Copy link
Member Author

Choose a reason for hiding this comment

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

haha, found this, yarnpkg/website#913 (comment)

arguing about this with Homebrew maintainers
This has already been done countless times, all linked throughout this thread

Yarn can be installed without node, but underlyingly, yarn does need node though.

Copy link
Member

Choose a reason for hiding this comment

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

Huh, interesting. I opened #90839, in case that's something we want to consider doing.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, looks good to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

You might also be interested about this nodejs TSC discussion, nodejs/TSC#1012

Basically moving forward, nodejs would have corepack serve as abstraction to talk to nodejs package manager like npm, yarn, pnpm.

ref https://nodejs.org/api/corepack.html

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I switched it back to yarn dependency.
Also, it seems we don't need python to build the formula.

@bayandin bayandin force-pushed the bump-opensearch-dashboards-1.2.0 branch from 6c67d67 to b4f5288 Compare December 9, 2021 11:27
@chenrui333 chenrui333 added the ready to merge PR can be merged once CI is green label Dec 9, 2021
carlocab added a commit to carlocab/homebrew-core that referenced this pull request Dec 9, 2021
@carlocab
yarn: make node a test dependency
e083362 
Yarn can be used with multiple versions of Node, but depending on `node`
specifically breaks that.

This change allows `yarn` to be used as a dependency for
`opensearch-dashboards` (cf. Homebrew#90403). See also yarnpkg/website#913.

While we're here, let's adjust the `NPM_CONFIG_PYTHON` value, since the
`node` formula doesn't even depend on a Python3 formula at runtime
anymore (and it's not that picky about the Python3 version anyway).
@carlocab carlocab mentioned this pull request Dec 9, 2021
Closed
6 tasks
@bayandin bayandin force-pushed the bump-opensearch-dashboards-1.2.0 branch from b4f5288 to 0c69e5f Compare December 9, 2021 23:14
BrewTestBot pushed a commit that referenced this pull request Dec 10, 2021
@carlocab @BrewTestBot
yarn: make node a test dependency
315664a 
Yarn can be used with multiple versions of Node, but depending on `node`
specifically breaks that.

This change allows `yarn` to be used as a dependency for
`opensearch-dashboards` (cf. #90403). See also yarnpkg/website#913.

While we're here, let's adjust the `NPM_CONFIG_PYTHON` value, since the
`node` formula doesn't even depend on a Python3 formula at runtime
anymore (and it's not that picky about the Python3 version anyway).

Closes #90839.

Signed-off-by: Sean Molenaar <1484494+SMillerDev@users.noreply.github.com>
Signed-off-by: BrewTestBot <1589480+BrewTestBot@users.noreply.github.com>
@bayandin bayandin force-pushed the bump-opensearch-dashboards-1.2.0 branch from 0c69e5f to 400fe18 Compare December 10, 2021 11:53
@branchvincent branchvincent mentioned this pull request Dec 10, 2021
Closed
19 tasks
@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

This was referenced Dec 10, 2021
Closed
Closed
@github-actions github-actions bot added the outdated PR was locked due to age label Jan 10, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 10, 2022
@chenrui333 chenrui333 deleted the bump-opensearch-dashboards-1.2.0 branch December 18, 2022 06:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@iMichka iMichka iMichka left review comments

@carlocab carlocab carlocab left review comments

@bayandin bayandin bayandin approved these changes

Assignees
No one assigned
Labels
bump-formula-pr PR was created using `brew bump-formula-pr` CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. nodejs Node or npm use is a significant feature of the PR or issue outdated PR was locked due to age ready to merge PR can be merged once CI is green
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@chenrui333 @BrewTestBot @bayandin @iMichka @carlocab