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

Updates to ethereumjs-config v2.0.0 (Cherry Pick Edition 🍒 ) #913

Merged
merged 38 commits into from
Oct 20, 2020

Conversation

holgerd77
Copy link
Member

Replacement PR for #886

PR cherry-picks the code logic commits from the original PR and add the linting runs as manual commits based upon dedicated npm run lint:fix runs. A rebase here was not possible due to the heavy changes along the road e.g. with the block refactor or account removal PRs.

This is WIP, linting is working (apart from one error which can be addressed later). Functionally equivalent currently up to 3b7e6ff (Ethash linting) with some additional cherry-picks from later commits.

@codecov
Copy link

codecov bot commented Oct 16, 2020

Codecov Report

Merging #913 into master will decrease coverage by 1.17%.
The diff coverage is 87.50%.

Impacted file tree graph

Flag Coverage Δ
#block 75.90% <83.33%> (-1.13%) ⬇️
#blockchain 80.33% <100.00%> (-0.84%) ⬇️
#common 92.03% <100.00%> (-1.16%) ⬇️
#ethash 82.08% <100.00%> (+0.27%) ⬆️
#tx 88.63% <100.00%> (-1.75%) ⬇️
#vm 86.89% <80.00%> (-1.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@holgerd77
Copy link
Member Author

holgerd77 commented Oct 16, 2020

Ok, this is now functionally equivalent to the original PR. 😄

It would be really nice if someone could pick this up later the day, I will continue a bit but stop soon and @evertonfraga might also not be available today and it would be really good if we can merge this soon.

An in-depth review here is not necessary any more, I already reviewed thoroughly by re-applying the original PR here. I would also pledge that we can merge this (so if you think this is ready: please do!) with the temporary ethereumjs-config branch packages still referenced. This is a little thing to update which we can do later and then we have this huge dependecy-creating PR out of the way and can do the finalization on small and a lot more comprehensible steps.

@evertonfraga
Copy link
Contributor

evertonfraga commented Oct 16, 2020

Thanks a million, @holgerd77.

To summarize the problem:

The old ESLint script had an assumption of a relative path from the package's package.json to the dependency, which is not always true in a monorepo setup (consider lerna hoisting, yarn workspaces, etc.)

The updated script works locally, but not on github actions.

Some options we have:

  1. ESLint: see if there's any way around it using (or removing) --resolve-plugins-relative-to.
  2. Make the path discovery function work with GH actions.

For testing in GH actions, I recommend:

  1. pushing to a new branch on ethereumjs-config each time, so you don't be subject to any kind of cache
  2. updating GitPkg references on our package.json files.

@holgerd77
Copy link
Member Author

holgerd77 commented Oct 19, 2020

Works, jay! 🎉

This is actually the first time I successfully battled with the monorepo CI, a bit proud. 😂

Now we just need to the the v2.0 config releases after merging ethereumjs/ethereumjs-config#39 and update the ethereumjs-config dependency references here and then we should be able to merge.

@holgerd77
Copy link
Member Author

Ok, this is ready now. Would be great if this can be reviewed and merged quickly (please just do), so that we have this out of the way and can go on with monorepo related work.

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

Okay, have glanced over this PR and do not see anything alarming. Approved! 😄

@holgerd77 holgerd77 merged commit 4be4159 into master Oct 20, 2020
@holgerd77 holgerd77 deleted the update-ethereumjs-config-rebased branch October 20, 2020 11:55
@holgerd77
Copy link
Member Author

@jochem-brouwer thanks! 😄

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

Successfully merging this pull request may close these issues.

3 participants