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

feat: sshnp_docker_demo #97

Merged
merged 87 commits into from
Jun 9, 2023
Merged

feat: sshnp_docker_demo #97

merged 87 commits into from
Jun 9, 2023

Conversation

JeremyTubongbanua
Copy link
Member

@JeremyTubongbanua JeremyTubongbanua commented May 29, 2023

What I did

  • demo of sshnp between two docker containers (+1 more for sshrvd)

closes #96

@JeremyTubongbanua JeremyTubongbanua self-assigned this May 29, 2023
@JeremyTubongbanua JeremyTubongbanua marked this pull request as ready for review May 29, 2023 20:30
@JeremyTubongbanua
Copy link
Member Author

JeremyTubongbanua commented May 29, 2023

@natt-n followed the README.md and got it working!

Copy link
Member

@cpswan cpswan left a comment

Choose a reason for hiding this comment

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

Rather than having fiddly instructions for people to specify their architecture the Dockerfiles should determine the architecture and act accordingly. When doing RUN commands this can generally be achieved with a case statement based on uname -m e.g. we use this for the Dockerfile.package in sshnoports:

RUN set -eux; \
    case "$(dpkg --print-architecture)" in \
        amd64) \
            ARCH="x64";; \
        armhf) \
            ARCH="arm";; \
        arm64) \
            ARCH="arm64";; \
        riscv64) \
            ARCH="riscv64";; \
    esac; \
    mkdir sshnp; \
    mkdir tarball; \
    dart pub get; \
    dart compile exe bin/activate_cli.dart -v -o sshnp/at_activate; \
    dart compile exe bin/sshnp.dart -v -o sshnp/sshnp; \
    dart compile exe bin/sshnpd.dart -v -o sshnp/sshnpd; \
    dart compile exe bin/sshrv.dart -v -o sshnp/sshrv; \
    dart compile exe bin/sshrvd.dart -v -o sshnp/sshrvd; \
    cp scripts/* sshnp; \
    tar -cvzf tarball/sshnp-linux-${ARCH}.tgz sshnp

It's also possible to use the TARGETPLATFORM ARG variable that gets passed in to a build like this:

ARG TARGETPLATFORM
RUN case ${TARGETPLATFORM} in \
         "linux/amd64")  ARCH=amd64  ;; \
         "linux/arm64")  ARCH=arm64  ;; \
         "linux/arm/v7") ARCH=armhf  ;; \
         "linux/arm/v6") ARCH=armel  ;; \
         "linux/386")    ARCH=i386   ;; \
    esac \
 && wget -q https://github.com/org/thing/releases/download/${VERSION}/whatever-${ARCH} -O /thing

More generally it's considered good practice to have multi line RUN statements rather than many of them, as each command in a Dockerfile creates a new layer in the resulting image.

@JeremyTubongbanua
Copy link
Member Author

@cpswan I have

  1. refactored README to use @XavierChanth's work with the sshnpd/sshnp installers
  2. sshnp_docker_demo_base GHA
  3. docker-build script tries to pull base image from docker hub
# try to docker pull, if it doesn't work, build local image
sudo docker pull $BASE_IMAGE_NAME
if [ $? -ne 0 ]
then
    echo "Could not pull $BASE_IMAGE_NAME. Building local image."
    sudo docker build -t $BASE_IMAGE_NAME -f ../demo-base/Dockerfile ../demo-base
fi
  1. Dockerfile no longer installs binaries (as installers do that for you)

Copy link
Member

@cpswan cpswan left a comment

Choose a reason for hiding this comment

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

Just a few nits, and then I think we're ready to merge

.github/workflows/sshnp_docker_demo_base.yml Outdated Show resolved Hide resolved
.github/workflows/sshnp_docker_demo_base.yml Outdated Show resolved Hide resolved
sshnp_docker_demo/demo-base/Dockerfile Outdated Show resolved Hide resolved
cpswan
cpswan previously approved these changes Jun 9, 2023
@JeremyTubongbanua JeremyTubongbanua merged commit 04dbdd3 into trunk Jun 9, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sshnp between 2 docker containers demo
3 participants