Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

fix: issue #310 #337

Merged
merged 8 commits into from
Aug 4, 2021
Merged

fix: issue #310 #337

merged 8 commits into from
Aug 4, 2021

Conversation

simonpfeifhofer
Copy link
Contributor

Closes: #310

Description

  • fix make target build-docker-local-ethermint
  • fix make target localnet-start
    • add ip-addresses for localnet-setup generation
    • add localnet-setup folder to keep localnet-setup-artifacts
    • map localnet-setup-artifacts to containers
    • change start-docker.sh to pick up generated localnet-setup-artifacts

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

- add ip-addresses for localnet-setup generation
- add localnet-setup folder to keep localnet-setup-artifacts
- map localnet-setup-artifacts to containers
- change start-docker.sh to pick up generated testnet artifacts
Copy link
Contributor

@leejw51crypto leejw51crypto left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Thanks! Looks like this only works if the repository is on the $GOPATH, can you fix that? Otherwise, LGTM

@simonpfeifhofer
Copy link
Contributor Author

Thanks! Looks like this only works if the repository is on the $GOPATH, can you fix that? Otherwise, LGTM

Thank you for the feedback.
Really? Tested it with the repository not on $GOPATH. Any hints for me to reproduce it? Happy to fix it.

@simonpfeifhofer
Copy link
Contributor Author

@fedekunze would be happy to get additional info about your finding.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@fedekunze
Copy link
Contributor

having some issues running make localnet-start

make localnet-start
docker-compose down
Removing network ethermint_localnet
WARNING: Network ethermint_localnet not found.
mkdir -p localnet-setup
docker build --tag ethermintd/node ../.. -f ethermintnode/Dockerfile
[+] Building 5.6s (15/15) FINISHED                                                                                                                                                                                                 
 => [internal] load build definition from Dockerfile                                                                                                                                                                          0.0s
 => => transferring dockerfile: 37B                                                                                                                                                                                           0.0s
 => [internal] load .dockerignore                                                                                                                                                                                             0.0s
 => => transferring context: 34B                                                                                                                                                                                              0.0s
 => [internal] load metadata for docker.io/library/golang:1.14                                                                                                                                                                0.6s
 => [internal] load metadata for docker.io/library/golang:stretch                                                                                                                                                             0.6s
 => [build-env 1/5] FROM docker.io/library/golang:stretch@sha256:9be20fec15ebf0f28d16b03cd93a2152513da0e85af45e3f3d3381deab818bf5                                                                                             0.0s
 => [final 1/5] FROM docker.io/library/golang:1.14@sha256:1a7173b5b9a3af3e29a5837e0b2027e1c438fd1b83bbee8f221355087ad416d6                                                                                                    0.0s
 => [internal] load build context                                                                                                                                                                                             4.8s
 => => transferring context: 7.65MB                                                                                                                                                                                           4.6s
 => CACHED [final 2/5] RUN apt-get update                                                                                                                                                                                     0.0s
 => CACHED [build-env 2/5] RUN apt-get update && apt-get upgrade -y &&     apt-get install -y curl make git libc-dev bash gcc                                                                                                 0.0s
 => CACHED [build-env 3/5] WORKDIR /go/src/github.com/tharsis/ethermint                                                                                                                                                       0.0s
 => CACHED [build-env 4/5] COPY . .                                                                                                                                                                                           0.0s
 => CACHED [build-env 5/5] RUN make build-linux                                                                                                                                                                               0.0s
 => CACHED [final 3/5] COPY --from=build-env /go/src/github.com/tharsis/ethermint/build/ethermintd /                                                                                                                          0.0s
 => CACHED [final 4/5] COPY --from=build-env /go/src/github.com/tharsis/ethermint/scripts/start-docker.sh /                                                                                                                   0.0s
 => exporting to image                                                                                                                                                                                                        0.0s
 => => exporting layers                                                                                                                                                                                                       0.0s
 => => writing image sha256:9def4ee4087246481945ad537bf7977f1fa55bc66cada66c44c59792f5375373                                                                                                                                  0.0s
 => => naming to docker.io/ethermintd/node                                                                                                                                                                                    0.0s

Use 'docker scan' to run Snyk tests against images to find vulnerabilities and learn how to fix them
if ! [ -f localnet-setup/node0/ethermintd/config/genesis.json ]; then docker run --rm -v /Users/federico/tharsis/ethermint/localnet-setup:/ethermint:Z ethermintd/node "./ethermintd testnet --v 4 -o /ethermint --keyring-backend=test --ip-addresses ethermintdnode0,ethermintdnode1,ethermintdnode2,ethermintdnode3"; fi
/bin/bash: ./ethermintd: cannot execute binary file: Exec format error
make: *** [localnet-start] Error 126

@simonpfeifhofer
Copy link
Contributor Author

@fedekunze thank you! Added 6d52409. Can you retry?

Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

tested ACK 💯

@fedekunze
Copy link
Contributor

@khoslaventures feel free to leave an additional review, or I can merge it by EOD, and then we can address your comments later on a follow-up PR 👍

@simonpfeifhofer
Copy link
Contributor Author

@khoslaventures apart from the Windows portions I tried to address your points. IMO we should remove the Windows parts. There are additional-points (a few already visible as issues #339, #370) which are necessary to fix if Windows should be fully supported. @leejw51crypto what do you think about that since you have created the issues? Is it an option to use WSL2?

@fedekunze fedekunze enabled auto-merge (squash) July 29, 2021 09:48
@fedekunze
Copy link
Contributor

@simonpfeifhofer can you pull the latest changes? I enabled auto-merge

@simonpfeifhofer
Copy link
Contributor Author

@simonpfeifhofer can you pull the latest changes? I enabled auto-merge

@fedekunze done.

@simonpfeifhofer
Copy link
Contributor Author

@fedekunze please let me know if there is still something which is missing in your opinion.

@fedekunze
Copy link
Contributor

@simonpfeifhofer I enabled auto-merge but it seems that you need to update your configuration as an external contributor to accept upstream rebasing (when the branch needs to be updated)

@fedekunze fedekunze added the automerge Automatically merge PR once all prerequisites pass. label Aug 4, 2021
@simonpfeifhofer
Copy link
Contributor Author

simonpfeifhofer commented Aug 4, 2021

@fedekunze: Because of the policy on the repository that out-of-date-branches cannot be merged it's necessary to update the branch every time a PR gets merged into main. I've done this three times. Unfortunately in the meantime other PRs have been merged. At the current point in time the branch is up to date, but the workflows need approval by a maintainer and if they complete successfully the PR will be auto-merged successfully. Do you refer to the setting Allow rebase merging with you need to update your configuration as an external contributor to accept upstream rebasing? If yes: That's already possible. What do I miss?

@fedekunze
Copy link
Contributor

@simonpfeifhofer I will add some changes to the contributor guidelines to prevent this from happening again. I was referring to this doc to allow changes from maintainers:

https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

@fedekunze fedekunze merged commit 300af0d into evmos:main Aug 4, 2021
yihuang referenced this pull request in yihuang/ethermint Sep 8, 2023
* Problem: no unit test for native action

Solution:
- add unit test

* test nested cases
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make target build-docker-local-ethermint
4 participants