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

Fixed e2e makefile target to use deploy-cli instead of deploy-script.js #78

Merged
merged 4 commits into from
Oct 4, 2024

Conversation

Jovonni
Copy link
Contributor

@Jovonni Jovonni commented Oct 3, 2024

Since deploy-contract.js is now "subsumed" by deploy-cli.js, we replace the Makefile target:

New command to deploy:

$(DEPLOY) src/builder/init-orca.js

which expands to:

npx --no-install tsx ../e2e-testing/scripts/deploy-cli.ts src/builder/init-orca.js

Fixes #77 , renders #64 outdated, and addresses #72

Shoutout to @dckc & @0xpatrickdev for the heads up!

@amessbee try this now, should be good 🚀

@Jovonni Jovonni self-assigned this Oct 3, 2024
@Jovonni Jovonni changed the title fix(Makefile): fixed e2e makefile target to use deploy-cli instead deploy-script.js fix(Makefile): fixed e2e makefile target to use deploy-cli instead of deploy-script.js Oct 3, 2024
@Jovonni Jovonni changed the title fix(Makefile): fixed e2e makefile target to use deploy-cli instead of deploy-script.js Fixed e2e makefile target to use deploy-cli instead of deploy-script.js Oct 3, 2024
@0xpatrickdev
Copy link
Member

Nice, would be great to close out #72 here or in a fast follow

@Jovonni Jovonni requested review from dckc and amessbee October 3, 2024 21:31
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

good stuff, but using npx really needs --no-install.

Otherwise, one typo and npx downloads and runs who-knows-what.

# todo: figure out why this sequence # is always off by 1 forcing a retry
# yarn node script/deploy-contract.js

npx tsx ../e2e-testing/scripts/deploy-cli.ts test/builder/init-orca.js
Copy link
Member

Choose a reason for hiding this comment

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

npx by itself is a bit of a loose cannon. Be sure to use --no-install.

Suggested change
npx tsx ../e2e-testing/scripts/deploy-cli.ts test/builder/init-orca.js
npx --no-install tsx ../e2e-testing/scripts/deploy-cli.ts test/builder/init-orca.js

consider factoring it out a la...

TSX=npx --no-install tsx

and then...

Suggested change
npx tsx ../e2e-testing/scripts/deploy-cli.ts test/builder/init-orca.js
$(TSX) ../e2e-testing/scripts/deploy-cli.ts test/builder/init-orca.js

Copy link
Member

Choose a reason for hiding this comment

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

you could even go further...

Suggested change
npx tsx ../e2e-testing/scripts/deploy-cli.ts test/builder/init-orca.js
$(DEPLOY) test/builder/init-orca.js

Copy link
Contributor Author

@Jovonni Jovonni Oct 3, 2024

Choose a reason for hiding this comment

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

ah good ideas! will do

@@ -259,26 +259,7 @@ test-orca:

# todo remove clean install steps after debugging
e2e:
make clean
Copy link
Member

Choose a reason for hiding this comment

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

👏 for replacing lots of old lines with 1 new one

@dckc
Copy link
Member

dckc commented Oct 3, 2024

How about deleting deploy-contract.js so that the next person doesn't trip over it?

@Jovonni Jovonni force-pushed the jo/e2e-deployment-fix branch from 84a5e1b to 1db90cf Compare October 4, 2024 10:58
@Jovonni Jovonni requested a review from dckc October 4, 2024 11:41
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

Nice!

Be sure to autosquash. I don't think we have bots for that here.

@Jovonni Jovonni force-pushed the jo/e2e-deployment-fix branch from c344d92 to da643e4 Compare October 4, 2024 14:09
# todo: figure out why this sequence # is always off by 1 forcing a retry
# yarn node script/deploy-contract.js

$(DEPLOY) src/builder/init-orca.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@Jovonni Jovonni merged commit cf0987f into main Oct 4, 2024
3 checks passed
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.

local deployment using deploy-contract.js no longer works
4 participants