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

e2e: chain initialization refactor for extensibility #1898

Merged
merged 10 commits into from
Jun 30, 2022

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Jun 29, 2022

Part of: #1879

What is the purpose of the change

Our current chain initialization design does not allow for spinning up single nodes. This functionality is useful for testing feautes like state-sync.

This PR introduced the barebones structure to initialize single nodes via Docker. To build a single node initialization image, run:

make docker-build-e2e-init-node

Additional information about the new design can be found in tests/e2e/initialization/README.md

Brief Changelog

  • move initialization logic to initialzation package
  • refactor init Dockerfile to support scripts provided by ARG
  • create a stub script for single node initialization

Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

Documentation and Release Note

@github-actions github-actions bot added C:docs Improvements or additions to documentation T:build labels Jun 29, 2022
@p0mvn p0mvn marked this pull request as ready for review June 30, 2022 00:00
@p0mvn p0mvn requested a review from a team June 30, 2022 00:00
Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

Personally untested, but all of the logic here is sound to me. Well done 👍

tests/e2e/containers/containers.go Show resolved Hide resolved
# Docker ARGs are not expanded in ENTRYPOINT in the exec mode. At the same time,
# it is impossible to add CMD arguments when running a container in the shell mode.
# As a workaround, we create the entrypoint.sh script to bypass these issues.
RUN echo "#!/bin/bash\n${E2E_SCRIPT_NAME} \"\$@\"" >> entrypoint.sh && chmod +x entrypoint.sh
Copy link
Member

Choose a reason for hiding this comment

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

For my learning, what are the semantics here \"\$@\""? I have never seen this before.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is bash syntax for expanding cmd line parameters.

Basically, since we want to call chain init and node init scripts by providing them with certain initialization arguments via Docker, we need to be able to expand them in the bash script.

This post points to a relevant man page about the syntax:
https://stackoverflow.com/questions/3898665/what-is-in-bash

tests/e2e/initialization/README.md Outdated Show resolved Hide resolved
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM aside from lack of udnerstanding the added main package

@p0mvn
Copy link
Member Author

p0mvn commented Jun 30, 2022

Thanks for the reviews, all comments are addressed so I'm going to merge this.

@p0mvn p0mvn merged commit 9f64027 into main Jun 30, 2022
@p0mvn p0mvn deleted the roman/e2e-chaininit-refactor branch June 30, 2022 03:07
p0mvn added a commit that referenced this pull request Jun 30, 2022
* refactor: e2e initialization package

* remove unused v10 logic

* md lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:docs Improvements or additions to documentation T:build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants