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

ci: Fix Docker options after removing oasis-deposit #471

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

abukosek
Copy link
Contributor

@abukosek abukosek commented Dec 12, 2024

Note that I haven't found a way to pass the script arguments to the end of the Docker run command from GitHub Actions, but the nice part is that they don't really matter, since the test mnemonic and 5 accounts is already the default 🤷‍♂️

Closes #465.

Copy link

netlify bot commented Dec 12, 2024

Deploy Preview for oasisprotocol-sapphire-paratime canceled.

Name Link
🔨 Latest commit 9541ab6
🔍 Latest deploy log https://app.netlify.com/sites/oasisprotocol-sapphire-paratime/deploys/6762ae93b012a700081d8240

@abukosek abukosek force-pushed the andrej/fix/update-ci-deposit branch 2 times, most recently from 2d84b1f to c3a27a7 Compare December 12, 2024 11:06
@abukosek abukosek marked this pull request as ready for review December 12, 2024 11:44
@abukosek abukosek force-pushed the andrej/fix/update-ci-deposit branch from c3a27a7 to 9541ab6 Compare December 18, 2024 11:14
@abukosek abukosek requested a review from matevz December 18, 2024 11:14
@matevz
Copy link
Member

matevz commented Dec 18, 2024

Can you pass the -to and -n inside the options: section? Or did you just hit actions/runner#1152 ?

@abukosek
Copy link
Contributor Author

Can you pass the -to and -n inside the options: section? Or did you just hit actions/runner#1152 ?

Yeah, I tried both options and args and neither worked, because in both cases the options are treated as arguments to the docker run command instead of as arguments to the script inside the container :(

@abukosek abukosek merged commit 295a7c2 into main Dec 18, 2024
12 checks passed
@matevz
Copy link
Member

matevz commented Dec 18, 2024

Can you pass the -to and -n inside the options: section? Or did you just hit actions/runner#1152 ?

Yeah, I tried both options and args and neither worked, because in both cases the options are treated as arguments to the docker run command instead of as arguments to the script inside the container :(

What about overriding --entrypoint '/start.sh -to 0x...' ?

@abukosek abukosek deleted the andrej/fix/update-ci-deposit branch December 18, 2024 14:41
@abukosek
Copy link
Contributor Author

Can you pass the -to and -n inside the options: section? Or did you just hit actions/runner#1152 ?

Yeah, I tried both options and args and neither worked, because in both cases the options are treated as arguments to the docker run command instead of as arguments to the script inside the container :(

What about overriding --entrypoint '/start.sh -to 0x...' ?

That is super-hacky and harder to maintain. In the case of our CI, funding 2 accounts vs. 5 doesn't result in a noticeable speed improvement to warrant the extra maintenance burden IMO :)

@matevz
Copy link
Member

matevz commented Dec 18, 2024

That is super-hacky and harder to maintain. In the case of our CI, funding 2 accounts vs. 5 doesn't result in a noticeable speed improvement to warrant the extra maintenance burden IMO :)

It is for cases when a specific dApp CI tests require non-default accounts to be funded, or none at all.

@abukosek
Copy link
Contributor Author

It is for cases when a specific dApp CI tests require non-default accounts to be funded, or none at all.

Yeah, in that case overriding the entrypoint is an option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update account funding parameters on CI
2 participants