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

chore(Dockerfile): Refactor dockerfile to Debian Slim with no deps #613

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

alfrunes
Copy link
Contributor

No description provided.

@mender-test-bot
Copy link

@alfrunes, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".


my commands and options

You can trigger a pipeline on multiple prs with:

  • mentioning me and start pipeline --pr mender/127 --pr mender-connect/255

You can start a fast pipeline, disabling full integration tests with:

  • mentioning me and start pipeline --fast

You can trigger GitHub->GitLab branch sync with:

  • mentioning me and sync

You can cherry pick to a given branch or branches with:

  • mentioning me and:
 cherry-pick to:
 * 1.0.x
 * 2.0.x

@alfrunes alfrunes requested a review from lluiscampos May 27, 2024 09:25
Copy link
Contributor

@lluiscampos lluiscampos left a comment

Choose a reason for hiding this comment

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

Okey, so we are finally going to publish mender-artifact on Docker too 👍

See below - we need to preserver the backwards compatibility build before merging this.

Dockerfile Outdated
@@ -1,9 +1,10 @@
# Keep golang version aligned with latest yocto release
Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed this in the past. We cannot silently update the golang version.

However the intention now is to actually publish this image, so we cannot anymore lock to golang 1.17 here (obviously). So could you please extend your PR (scope creep coming!) with the following:

  • Add a new Dockerfile (f.eks Dockerfile.backcompat or something like that) which still build the binary with golang 1.17 and keeps this comment
  • Add a new CI job to just do docker build on this new Dockerfile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having an extra Dockerfile for building for go1.17 seems a bit excessive. How about a simple gitlab job (build:yoctocompat) that runs go build in a golang:1.17 container?

Copy link
Contributor

Choose a reason for hiding this comment

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

Having an extra Dockerfile for building for go1.17 seems a bit excessive. How about a simple gitlab job (build:yoctocompat) that runs go build in a golang:1.17 container?

Actually I thought about that during this week of inactivity in this PR. It is not only better, but also aligned to other client repos when we had to do backwards compatibility jobs. Thanks

Copy link
Contributor

@lluiscampos lluiscampos left a comment

Choose a reason for hiding this comment

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

Thank you! I am very happy to see this coming up together 🚀

Pre-approving, but please see my comments below

.gitlab-ci.yml Outdated
stage: build
before_script:
- docker login --username "$CI_REGISTRY_USER" --password "$CI_REGISTRY_PASSWORD" "$CI_REGISTRY"
Copy link
Contributor

Choose a reason for hiding this comment

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

If the password is not masked, this line would leak it. It is better to use

Suggested change
- docker login --username "$CI_REGISTRY_USER" --password "$CI_REGISTRY_PASSWORD" "$CI_REGISTRY"
- echo ${CI_REGISTRY_PASSWORD} | docker login --username ${CI_REGISTRY_USER} --password-stdin ${CI_REGISTRY}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I get it. Isn't it the same with the echo command? The variable is expanded there as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

It won't be logged into stdout - (iow the CI logs). I cannot link to an LFF - but search for "GitLab CI secrets leak" in the LFF archive :)

.gitlab-ci.yml Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
.gitlab-ci.yml Show resolved Hide resolved
Needs to be overwritten in some circumstances, for example when building
with coverage instrumentation.

Signed-off-by: Alf-Rune Siqveland <[email protected]>
@alfrunes
Copy link
Contributor Author

@lluiscampos I'm wondering if we should bump the build:make job to go1.23.4 since these binaries are not used inside Yocto. Otherwise, I'd remove the build:yoctocompat job since it would otherwise not be needed. What do you think?

@lluiscampos
Copy link
Contributor

@lluiscampos I'm wondering if we should bump the build:make job to go1.23.4 since these binaries are not used inside Yocto. Otherwise, I'd remove the build:yoctocompat job since it would otherwise not be needed. What do you think?

Yes, there is no reason to keep build:make in older golang.

As a rule of thumb, we should have use golang versions: one specific for the new build:yoctocompat job (go 1.17, although in a matter of weeks we will be able to upgrade!), and then the latest for every other job and Dockerfiles 👍

@alfrunes
Copy link
Contributor Author

@lluiscampos I'm wondering if we should bump the build:make job to go1.23.4 since these binaries are not used inside Yocto. Otherwise, I'd remove the build:yoctocompat job since it would otherwise not be needed. What do you think?

Oops, sorry (apparently my future self was talking here). I meant go1.22.4 of course 😅

Yes, there is no reason to keep build:make in older golang.

As a rule of thumb, we should have use golang versions: one specific for the new build:yoctocompat job (go 1.17, although in a matter of weeks we will be able to upgrade!), and then the latest for every other job and Dockerfiles 👍

Agreed! I'll update the build:make job as well 👍

@alfrunes alfrunes force-pushed the Dockerfile branch 3 times, most recently from 4ffe851 to 3818d08 Compare June 11, 2024 12:05
@alfrunes
Copy link
Contributor Author

@mender-test-bot start pipeline

@mender-test-bot
Copy link

Hello 😺 I created a pipeline for you here: Pipeline-1327329596

Build Configuration Matrix

Key Value
AUDITLOGS_REV master
BUILD_BEAGLEBONEBLACK true
BUILD_CLIENT true
BUILD_QEMUX86_64_BIOS_GRUB true
BUILD_QEMUX86_64_BIOS_GRUB_GPT true
BUILD_QEMUX86_64_UEFI_GRUB true
BUILD_VEXPRESS_QEMU true
BUILD_VEXPRESS_QEMU_FLASH true
BUILD_VEXPRESS_QEMU_UBOOT_UEFI_GRUB true
CREATE_ARTIFACT_WORKER_REV master
DEPLOYMENTS_ENTERPRISE_REV master
DEPLOYMENTS_REV master
DEVICEAUTH_ENTERPRISE_REV master
DEVICEAUTH_REV master
DEVICECONFIG_REV master
DEVICECONNECT_REV master
DEVICEMONITOR_REV master
GENERATE_DELTA_WORKER_REV master
GUI_REV master
INTEGRATION_REV master
INVENTORY_ENTERPRISE_REV master
INVENTORY_REV master
IOT_MANAGER_REV master
MENDER_ARTIFACT_REV pull/613/head
MENDER_BINARY_DELTA_REV master
MENDER_CLI_REV master
MENDER_CONFIGURE_MODULE_REV master
MENDER_CONNECT_REV master
MENDER_CONVERT_REV master
MENDER_GATEWAY_REV master
MENDER_REV master
MENDER_SETUP_REV master
MENDER_SNAPSHOT_REV master
MONITOR_CLIENT_REV master
RUN_BACKEND_INTEGRATION_TESTS true
RUN_INTEGRATION_TESTS true
TENANTADM_REV master
TEST_QEMUX86_64_BIOS_GRUB true
TEST_QEMUX86_64_BIOS_GRUB_GPT true
TEST_QEMUX86_64_UEFI_GRUB true
TEST_VEXPRESS_QEMU true
TEST_VEXPRESS_QEMU_FLASH true
TEST_VEXPRESS_QEMU_UBOOT_UEFI_GRUB true
USERADM_ENTERPRISE_REV master
USERADM_REV master
WORKFLOWS_ENTERPRISE_REV master
WORKFLOWS_REV master

@@ -150,8 +150,6 @@ test:smoketests:linux:
- ./mender-artifact-linux read test-rfs.mender
- ./mender-artifact-linux validate test-rfs.mender
- ./tests/test_compressions/test_supported_compressions.sh
# QA-507: lock mender-artifact to OpenSSL 1.1
- ldd ./mender-artifact-linux | grep libssl.so.1.1
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not break compatibility this way. We need to plan it a bit. As discussed in person, we could offer two flavors in our Downloads page: one statically linked without pkcs11 and one dynamically linked for openssl3.

As this starts to be bigger than a nice to have PR, can we create a task for it? It involves the work here, plus having some sort of subdirs structure in the S3 bucket, and mender-docs.

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.

3 participants