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

Fix dangling modified file on make devnet-up #7051

Conversation

jyellick
Copy link
Contributor

When starting from a fresh clone of the repo and running

make
make cannon-prestate
make devnet-up

We are left with a file changed in the tree at

packages/contracts-bedrock/deploy-config/devnetL1.json

This file is a bit annoying as new changes to it should generally not be checked in but shows as an outstanding change. The python script reads the original file, backs it up, then rewrites that file with a new timestamp.

I'm not entirely certain why the timestamp needs to be rewritten, so I've left that modification in place. Instead, this change takes the approach of moving the original file to a 'template' path, and ignoring the modified file. In this way there's no longer any need to constantly create a backup and have the changes show up in a git diff.

If we don't like this approach, I'm happy to implement something else, just wanting to stop accidentally capturing this file in my changesets.

@jyellick jyellick requested review from a team as code owners August 29, 2023 17:35
@jyellick jyellick requested a review from Inphi August 29, 2023 17:35
@mergify mergify bot added the A-pkg-contracts-bedrock Area: packages/contracts-bedrock label Aug 29, 2023
Copy link
Contributor

@Inphi Inphi left a comment

Choose a reason for hiding this comment

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

Thanks!

@Inphi
Copy link
Contributor

Inphi commented Aug 29, 2023

@jyellick could you rerun the failing circle ci check - https://app.circleci.com/pipelines/github/bobanetwork/v3-anchorage/446/workflows/f89a47a6-4e1a-4870-af05-383b7d775d47/jobs/18765. The PR passes for me locally so this could just be a flake.

@mergify
Copy link
Contributor

mergify bot commented Aug 29, 2023

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

@mergify mergify bot added the S-conflict Status: A conflict is present label Aug 29, 2023
@jyellick jyellick force-pushed the cleanup-devnet-modified-file branch from c0cb422 to 89cad4d Compare August 30, 2023 13:16
@mergify mergify bot removed the S-conflict Status: A conflict is present label Aug 30, 2023
@jyellick jyellick force-pushed the cleanup-devnet-modified-file branch from 89cad4d to 3384fbe Compare August 30, 2023 16:00
@jyellick jyellick marked this pull request as draft August 30, 2023 16:48
@jyellick
Copy link
Contributor Author

This change seems to make the devnet CI a bit flaky, going to investigate, will undraft once ready.

@jyellick jyellick force-pushed the cleanup-devnet-modified-file branch 2 times, most recently from 1931da5 to e867f49 Compare August 30, 2023 20:09
When starting from a fresh clone of the repo and running

```
make
make cannon-prestate
make devnet-up
```

We are left with a file changed in the tree at

```
packages/contracts-bedrock/deploy-config/devnetL1.json
```

This file is a bit annoying as it should not be checked in but shows as
an outstanding change.  The python script reads the original file, backs
it up, then rewrites that file with a new timestamp.

I'm not entirely certain why the timestamp needs to be rewritten, so
I've left that modification in place.  Instead, this change takes the
approach of moving the original file to a 'template' path, and ignoring
the modified file.  In this way there's no longer any need to constantly
create a backup and have the changes show up in a git diff.
@jyellick jyellick force-pushed the cleanup-devnet-modified-file branch from e867f49 to 208731a Compare August 31, 2023 16:57
@jyellick jyellick marked this pull request as ready for review August 31, 2023 19:35
@mergify
Copy link
Contributor

mergify bot commented Aug 31, 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 31, 2023
@jyellick
Copy link
Contributor Author

Modified the change a bit to ensure that a version with the original timestamp is written during the initial alloc, while an updated timestamp is written during the devnet-up. I'm not entirely sure why this behavior is necessary, but, it seems to make CI happy.

@mergify
Copy link
Contributor

mergify bot commented Aug 31, 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 31, 2023
@jyellick
Copy link
Contributor Author

jyellick commented Sep 1, 2023

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Sep 1, 2023

requeue

❌ Command disallowed due to command restrictions in the Mergify configuration.

  • sender-permission>=write

@mergify
Copy link
Contributor

mergify bot commented Sep 7, 2023

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

@mergify mergify bot added the S-conflict Status: A conflict is present label Sep 7, 2023
@OptimismBot OptimismBot merged commit 2ab81ce into ethereum-optimism:develop Sep 7, 2023
60 of 70 checks passed
@mergify mergify bot removed the S-conflict Status: A conflict is present label Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-pkg-contracts-bedrock Area: packages/contracts-bedrock
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants