-
Notifications
You must be signed in to change notification settings - Fork 91
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
Add extra binaries in the setup command #1208
Conversation
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.
Nice! I wonder if there could be some option/way to install all the binaries without specifying them? Otherwise users have to type all this:
node dist/cli.js setup polkadot polkadot-parachain polkadot-prepare-worker polkadot-execute-worker
Just a suggestion, not sure if it makes sense.
I'm not sure that this is a very famous option (install all possibilities for a test) 🤔 |
Thanks @wirednkod, I think we should download |
@pepoviola Yeah, actually the worker binaries should always be downloaded along with |
Great, @wirednkod can you update the pr so we always download the |
…rkers; add 'all' command
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.
Very cool, I love the way you did it. setup polkadot polkadot-parachain
will continue to grab all the binaries so nothing breaks for anyone. 😀
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.
Nice! Just one comment, and I think one of my earlier comments was missed
@pepoviola I made this ready for review but it should not be merged, until the worker binaries are added to at least 1 release and I run some more tests against those; |
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.
🥳
@@ -209,6 +263,12 @@ const latestPolkadotReleaseURL = async ( | |||
return Boolean(obj); | |||
}); | |||
|
|||
if (!release) { | |||
console.log( | |||
`In repo '${repo}', there is no release for: '${name}'! Exiting...`, |
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.
Do we need to throw
here, since the next line
const { tag_name } = release;
will generate an error.
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.
Ah - good catch. I meant to throw error here - but I never did. Thank you
@@ -126,7 +126,7 @@ program | |||
"<binaries...>", | |||
`the binaries that you want to be downloaded, provided in a row without any separators;\nThey are downloaded in current directory and appropriate executable permissions are assigned.\nPossible options: 'polkadot', 'polkadot-parachain'\n${decorators.blue( | |||
"zombienet setup polkadot polkadot-parachain", | |||
)}`, | |||
)}\nNote: Downloading 'polkadot' downloads also 'polkadot-prepare-worker' and 'polkadot-execute-worker'`, |
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.
package polkadot into npm. inventing own package manager is dead end.
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.
@dzmitry-lahoda can you elaborate a bit more on this - what exactly you mean?
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.
Maybe he means that zombienet setup
is doing a job that can already be done by a package manager?
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.
If that is the case then i think it is not correct; Cause not everyone is using zombienet by cloning the repo; There are cases of using it as a release - and thus the release can help setting up the binaries;
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.
yes. just ask user to use apt npm nix curl dpkg etc.
adhoc pm is dead end. let start that it is not secure nor it will work on proxyvpnedfirewalled envs.
parity to release apt or and npm.
parity already releases nix polkadot. waiting for nix common good nodes;
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.
just ask user to install, like you ask to install k8s, podman, etc.
on windows can use scoop or winget.
on mac brew or nix.
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.
This was created as an easy way for non-advanced linux users to crash-test zombienet; It retrieves these from the release packages of gitlab and its not the "mandatory way to go";
I am fine for removing it if @pepoviola agrees - I do not see any value though on removing this;
ping @wirednkod this is ready to review/merge? would be good to include in the next release |
Just tested and make some minor changes. Everything works as expected. Thank you @mrcnski @pepoviola |
Missing task: When first time binaries are created during release - these binaries should be investigated and added to the setup process
Fixes #1202
Fixes #1209