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: move devnet test to make target #6235

Merged

Conversation

jyellick
Copy link
Contributor

@jyellick jyellick commented Jul 6, 2023

When encountering devnet test failures in CI, they're challenging to resolve and reproduce locally because the commands are embedded into the CI configuration and not the Makefile.

This PR simply extracts the commands from the CI steps and embeds them into the devnet python module, allowing for the python script to be invoked with a new --test flag to execute those steps. It also adds a devnet-test make target to allow simple invocation of the script in test mode.

@jyellick jyellick requested review from a team as code owners July 6, 2023 17:39
@changeset-bot
Copy link

changeset-bot bot commented Jul 6, 2023

⚠️ No Changeset found

Latest commit: ee5ccb1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link

netlify bot commented Jul 6, 2023

Deploy Preview for opstack-docs canceled.

Name Link
🔨 Latest commit 9b90e68
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/64bfe64a8409f200083f97a7

Copy link
Contributor

@tynes tynes left a comment

Choose a reason for hiding this comment

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

Generally in favor of the idea, not sure how I feel about the approach but could be open to it

bedrock-devnet/devnet/__init__.py Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
@jyellick
Copy link
Contributor Author

jyellick commented Jul 6, 2023

Generally in favor of the idea, not sure how I feel about the approach but could be open to it

I'm certainly happy to rework in some other way. As best as I could tell, the assorted build scripts are being migrated to python (vs. bash), so I tried to keep everything in the python, even though it's simply executing commands.

If there's a different preferred approach, please let me know. My goal here is primarily to allow developers to iterate while running tests without having to push it out and wait on CI.

@tynes
Copy link
Contributor

tynes commented Jul 9, 2023

Generally in favor of the idea, not sure how I feel about the approach but could be open to it

I'm certainly happy to rework in some other way. As best as I could tell, the assorted build scripts are being migrated to python (vs. bash), so I tried to keep everything in the python, even though it's simply executing commands.

If there's a different preferred approach, please let me know. My goal here is primarily to allow developers to iterate while running tests without having to push it out and wait on CI.

I am generally on board with this change after thinking about it more

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

Love this change - moving stuff from .circleci/config.yml into makefiles is a long term goal. Ideally we'd also move the devnet (with genesis contracts) tests as well, though that could be a separate PR as this is definitely an improvement.

If we do move the genesis tests, it would be great to remove the duplication as well so make devnet-test could just work for both genesis and deploy modes. I think the only difference is if --l1-contracts-json-path is specified or not so possibly it could switch based on whether that file exists or not.

@github-actions
Copy link
Contributor

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Jul 25, 2023
@jyellick jyellick force-pushed the ci-move-devnet-test-to-target branch from 9210063 to 9b90e68 Compare July 25, 2023 15:12
@jyellick
Copy link
Contributor Author

I've updated the --test path of the python to add or omit the sdk address path depending on whether the file exists. Now, things should work both for make devnet-up and make devnet-up-deploy -- I've also updated the circle CI definition to use it in both paths.

@github-actions github-actions bot removed the Stale label Jul 26, 2023
@jyellick
Copy link
Contributor Author

jyellick commented Aug 2, 2023

Is there anything preventing this from merging? Happy to make any desired changes if so.

@mergify
Copy link
Contributor

mergify bot commented Aug 3, 2023

Hey @jyellick! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Aug 3, 2023
@jyellick jyellick force-pushed the ci-move-devnet-test-to-target branch from 9b90e68 to 0f11efe Compare August 3, 2023 20:50
@jyellick jyellick requested a review from a team as a code owner August 3, 2023 20:50
@jyellick jyellick requested a review from clabby August 3, 2023 20:50
@mergify mergify bot removed the conflict label Aug 3, 2023
@mergify
Copy link
Contributor

mergify bot commented Aug 7, 2023

Hey @jyellick! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Aug 7, 2023
@jyellick jyellick force-pushed the ci-move-devnet-test-to-target branch from 0f11efe to ee5ccb1 Compare August 8, 2023 02:32
@mergify mergify bot removed the conflict label Aug 8, 2023
@jyellick jyellick force-pushed the ci-move-devnet-test-to-target branch from ee5ccb1 to 5ea42fe Compare August 15, 2023 14:09
@jyellick
Copy link
Contributor Author

Rebased to try to keep this current (though no conflicts existed). Any updates from the maintainers? @tynes @ajsutton ?

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

LGTM, other than I think we want to keep the capturing of op-node logs. I'm not an infra reviewer though.

.circleci/config.yml Outdated Show resolved Hide resolved
@ajsutton
Copy link
Contributor

Oh turns out I am an infra reviewer. :) @jyellick Would you mind restoring the capturing of op-node logs and then I'll get this merged.

Thank you so much for your incredible patience - we've been really slow on a bunch of your PRs.

When encountering devnet test failures in CI, they're challenging to
resolve and reproduce locally because the commands are embedded into the
CI configuration and not the Makefile.
@jyellick jyellick force-pushed the ci-move-devnet-test-to-target branch from 5ea42fe to f9a2c14 Compare August 17, 2023 14:04
@mergify mergify bot added the M-ci Meta: ci related work label Aug 17, 2023
@ajsutton
Copy link
Contributor

@jyellick The world is apparently out to get us today - the devnet build failed because of a network error downloading node. Can you kick it to rerun and then I think we should be good to go. It won't let me rerun because it look like its running in your circle ci org.

@mergify
Copy link
Contributor

mergify bot commented Aug 17, 2023

This PR has been added to the merge queue, and will be merged soon.

@mergify mergify bot added the S-on-merge-train Status: This PR is in the merge queue label Aug 17, 2023
@jyellick
Copy link
Contributor Author

Thanks @ajsutton, looks like the re-run fixed things, very happy to see this merge!

@mergify
Copy link
Contributor

mergify bot commented Aug 17, 2023

Hey @jyellick, this pull request failed to merge and has been dequeued from the merge train. If you believe your PR failed in the merge train because of a flaky test, requeue it by commenting with @mergifyio requeue.
More details can be found on the Queue: Embarked in merge train check-run.

@mergify mergify bot removed the S-on-merge-train Status: This PR is in the merge queue label Aug 17, 2023
@ajsutton ajsutton merged commit dfb2e87 into ethereum-optimism:develop Aug 17, 2023
62 of 72 checks passed
@jyellick jyellick deleted the ci-move-devnet-test-to-target branch September 1, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-ci Meta: ci related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants