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

fix(governance,networks): remove localhost from networks (again) #13321

Merged
merged 28 commits into from Mar 20, 2024
Merged

fix(governance,networks): remove localhost from networks (again) #13321

merged 28 commits into from Mar 20, 2024

Conversation

clemsos
Copy link
Member

@clemsos clemsos commented Feb 6, 2024

Description

This adds a fix to remove localhost from our networks package

Issues

Fixes #13307
Refs #

Checklist:

  • 1 PR, 1 purpose: my Pull Request applies to a single purpose
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the docs to reflect my changes if applicable
  • I have added tests (and stories for frontend components) that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • If my code involves visual changes, I am adding applicable screenshots to this thread

Release Note Draft Snippet

@cla-bot cla-bot bot added the cla-signed label Feb 6, 2024
@clemsos clemsos requested a review from julien51 February 6, 2024 12:24
julien51
julien51 reviewed Mar 19, 2024
View reviewed changes
packages/networks/bin/localhost.js
}

// log for debug purposes
console.log(localhost)
Copy link
Member

Choose a reason for hiding this comment

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

Do you wnat to keep that?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes lets keep that one so we have it to debug on CI

julien51
julien51 approved these changes Mar 19, 2024
View reviewed changes
Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

Minor things to fix/clarify!

packages/networks/bin/localhost.js Outdated Show resolved Hide resolved
packages/networks/bin/localhost.js
@@ -0,0 +1,83 @@
import fs from 'fs-extra'
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a quick note in the README how the localhost network is different from he other ones?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep added

@julien51 julien51 mentioned this pull request Mar 19, 2024
Merged
6 tasks
clemsos
clemsos commented Mar 20, 2024
View reviewed changes
tests/package.json Outdated Show resolved Hide resolved
clemsos
clemsos commented Mar 20, 2024
View reviewed changes
tests/bin/prepare.sh Outdated Show resolved Hide resolved
@clemsos clemsos merged commit dfa3e48 into master Mar 20, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julien51 julien51 julien51 approved these changes

Assignees
No one assigned
Labels
cla-signed run-all-tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Do not publish localhost network in npm package
2 participants
@clemsos @julien51