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.

Sign up for GitHub

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

Jump to bottom

chore!: clean up tx sim docker usage #3902

Merged
merged 6 commits into from
Sep 25, 2024
Merged

chore!: clean up tx sim docker usage #3902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
eb82ca8
chore!: clean up tx sim docker usage
cmwaters Sep 24, 2024
a02d91d
fix makefile
cmwaters Sep 24, 2024
71b686e
modify lint workflow
cmwaters Sep 24, 2024
c0ab4bc
rename back to hadolint
cmwaters Sep 24, 2024
cdfde3a
Merge branch 'main' into cal/tx-sim-docker
cmwaters Sep 25, 2024
29ade51
Merge branch 'main' into cal/tx-sim-docker
cmwaters Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/docker-build-publish.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ jobs:
packages: write
uses: celestiaorg/.github/.github/workflows/[email protected]
with:
dockerfile: Dockerfile
dockerfile: docker/Dockerfile
checkout_ref: ${{ github.event.inputs.ref }}
secrets: inherit

Expand All @@ -33,7 +33,7 @@ jobs:
packages: write
uses: celestiaorg/.github/.github/workflows/[email protected]
with:
dockerfile: docker/Dockerfile_txsim
dockerfile: docker/txsim/Dockerfile
packageName: txsim
checkout_ref: ${{ github.event.inputs.ref }}
secrets: inherit
2 changes: 2 additions & 0 deletions .github/workflows/lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ jobs:
# hadolint lints the Dockerfile
hadolint:
uses: celestiaorg/.github/.github/workflows/[email protected]
with:
dockerfile: "docker/Dockerfile"
Comment on lines +34 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Additional Dockerfile Detected for Linting

The workflow currently lints docker/Dockerfile. However, an additional Dockerfile docker/txsim/Dockerfile was found. To ensure comprehensive linting, please update the workflow to include this Dockerfile.

  • Update the hadolint job to include docker/txsim/Dockerfile.
  • Consider using a loop or multiple with clauses if supported by the workflow configuration.
🔗 Analysis chain

LGTM! Consider linting additional Dockerfiles.

The change correctly updates the Dockerfile path for the hadolint job, aligning with the PR objective of relocating Dockerfiles to a dedicated docker directory. This ensures that the linting process targets the correct Dockerfile after the relocation.

To ensure comprehensive linting coverage, please verify if there are other Dockerfiles (such as the one for txsim) that also need linting. If so, consider adding them to this workflow or creating separate linting jobs for them.

Run the following script to check for additional Dockerfiles:

If additional Dockerfiles are found, consider updating the workflow to lint them as well.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all Dockerfiles in the repository

# Test: Search for Dockerfiles. Expect: List of all Dockerfiles in the repository.
fd Dockerfile

Length of output: 57


yamllint:
runs-on: ubuntu-latest
Expand Down
9 changes: 5 additions & 4 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,13 @@ proto-format:
## build-docker: Build the celestia-appd docker image from the current branch. Requires docker.
build-docker:
@echo "--> Building Docker image"
$(DOCKER) build -t celestiaorg/celestia-app -f Dockerfile .
$(DOCKER) build -t celestiaorg/celestia-app -f docker/Dockerfile .
.PHONY: build-docker

## build-ghcr-docker: Build the celestia-appd docker image from the last commit. Requires docker.
build-ghcr-docker:
@echo "--> Building Docker image"
$(DOCKER) build -t ghcr.io/celestiaorg/celestia-app:$(COMMIT) -f Dockerfile .
$(DOCKER) build -t ghcr.io/celestiaorg/celestia-app:$(COMMIT) -f docker/Dockerfile .
.PHONY: build-ghcr-docker

## publish-ghcr-docker: Publish the celestia-appd docker image. Requires docker.
Expand All @@ -106,7 +106,8 @@ lint:
@echo "--> Running markdownlint"
@markdownlint --config .markdownlint.yaml '**/*.md'
@echo "--> Running hadolint"
@hadolint Dockerfile
@hadolint docker/Dockerfile
@hadolint docker/txsim/Dockerfile
@echo "--> Running yamllint"
@yamllint --no-warnings . -c .yamllint.yml
.PHONY: lint
Expand Down Expand Up @@ -186,7 +187,7 @@ txsim-build:

## txsim-build-docker: Build the tx simulator Docker image. Requires Docker.
txsim-build-docker:
docker build -t ghcr.io/celestiaorg/txsim -f docker/Dockerfile_txsim .
docker build -t ghcr.io/celestiaorg/txsim -f docker/txsim/Dockerfile .
.PHONY: txsim-build-docker

## adr-gen: Download the ADR template from the celestiaorg/.github repo.
Expand Down
0 Dockerfile → docker/Dockerfile
View file
Open in desktop
File renamed without changes.
93 changes: 0 additions & 93 deletions docker/txsim.sh
View file
Open in desktop

This file was deleted.

15 changes: 5 additions & 10 deletions docker/Dockerfile_txsim → docker/txsim/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Stage 1: generate celestia-appd binary
# Stage 1: generate txsim binary
FROM --platform=$BUILDPLATFORM docker.io/golang:1.22.6-alpine3.19 as builder

ARG TARGETOS
Expand All @@ -16,11 +16,10 @@ RUN apk update && apk add --no-cache \
musl-dev
COPY . /celestia-app
WORKDIR /celestia-app
# we need the celestia-appd build as we might want to create an account
# internally for txsimulation

RUN uname -a &&\
CGO_ENABLED=${CGO_ENABLED} GOOS=${TARGETOS} GOARCH=${TARGETARCH} \
make build && make txsim-build
make txsim-build

# Stage 2: create a minimal image with the binary
FROM docker.io/alpine:3.20
Expand All @@ -45,18 +44,14 @@ RUN apk update && apk add --no-cache \
-s /sbin/nologin \
-u ${UID}

# Copy in the celestia-appd binary
COPY --from=builder /celestia-app/build/celestia-appd /bin/celestia-appd
# Copy in the txsim binary
COPY --from=builder /celestia-app/build/txsim /bin/txsim

COPY --chown=${USER_NAME}:${USER_NAME} docker/txsim.sh /opt/entrypoint.sh
COPY --chown=${USER_NAME}:${USER_NAME} docker/txsim/entrypoint.sh /opt/entrypoint.sh

USER ${USER_NAME}

# Set the working directory to the home directory.
WORKDIR ${CELESTIA_HOME}

# grpc, rpc, api ports
EXPOSE 26657 1317 9090

ENTRYPOINT [ "/bin/bash", "/opt/entrypoint.sh" ]
0 docker/README.md → docker/txsim/README.md
View file
Open in desktop
File renamed without changes.
7 changes: 7 additions & 0 deletions docker/txsim/entrypoint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#!/bin/bash

echo "Starting txsim with command:"
echo "/bin/txsim $@"
echo ""

Comment on lines +3 to +6
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix argument handling in echo statement.

The current usage of $@ in the echo statement may lead to unexpected behavior if arguments contain spaces or special characters.

Apply this diff to fix the argument handling:

-echo "/bin/txsim $@"
+echo "/bin/txsim $*"

This change ensures that all arguments are properly represented as a single string when echoed.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
echo "Starting txsim with command:"
echo "/bin/txsim $@"
echo ""
echo "Starting txsim with command:"
echo "/bin/txsim $*"
echo ""
🧰 Tools
Shellcheck

[error] 4-4: Argument mixes string and array. Use * or separate argument.

(SC2145)

exec /bin/txsim $@
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Quote arguments in exec command.

The current usage of $@ in the exec command may lead to unexpected behavior if arguments contain spaces or special characters.

Apply this diff to fix the argument handling:

-exec /bin/txsim $@
+exec /bin/txsim "$@"

This change ensures that all arguments are properly passed to the txsim binary, preserving spaces and special characters.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
exec /bin/txsim $@
exec /bin/txsim "$@"
🧰 Tools
Shellcheck

[error] 7-7: Double quote array expansions to avoid re-splitting elements.

(SC2068)

13 changes: 6 additions & 7 deletions test/e2e/testnet/txsimNode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,12 @@ func CreateTxClient(
return nil, err
}
args := []string{
fmt.Sprintf("-k %d", 0),
fmt.Sprintf("-g %s", endpoint),
fmt.Sprintf("-t %ds", pollTime),
fmt.Sprintf("-b %d ", sequences),
fmt.Sprintf("-d %d ", seed),
fmt.Sprintf("-a %d ", blobsPerSeq),
fmt.Sprintf("-s %s ", blobRange),
fmt.Sprintf("-grpc-endpoint %s", endpoint),
fmt.Sprintf("-poll-time %ds", pollTime),
fmt.Sprintf("-seed %d ", seed),
fmt.Sprintf("-blob %d ", sequences),
fmt.Sprintf("-blob-amounts %d ", blobsPerSeq),
fmt.Sprintf("-blob-sizes %s ", blobRange),
}

if err := txIns.Build().SetArgs(args...); err != nil {
Expand Down
Loading