search

open-quantum-safe / liboqs-go

Go bindings for liboqs
https://openquantumsafe.org/
MIT License
69 stars 24 forks source link

Dev: Poc update docker build steps with buildx #20

Closed SamYuan1990 closed 1 year ago

SamYuan1990 commented 1 year ago

this is poc of #19

  1. as some package depends or package version is changed, hence I just ref https://github.com/open-quantum-safe/liboqs#linuxmacos to refined apt install steps.
  2. as for buildx it takes a lot of time, hence .... just poc for now.
  3. as I don't have token/apikey to push image, hence in this PR just build image without push it.
SamYuan1990 commented 1 year ago

poc with github action can be found here https://github.com/SamYuan1990/liboqs-go/actions/runs/4211312596/jobs/7309576931

SamYuan1990 commented 1 year ago

Thanks very much for this draft/poc PR! Looks pretty good already. See the single comments for "point improvements".

As per your leading comments

as for buildx it takes a lot of time, hence .... just poc for now.

Is this a real issue? It completes within an hour and builds for 3 platforms, if I see things right -- so arguably OK as far as I can see. But does this (ARM64?) run OK on M1?

as I don't have token/apikey to push image, hence in this PR just build image without push it.

Could you test this out with(in) your own docker hub namespace & API key? For CircleCI we set the variables "TARGETNAME" to point to the docker hub (target) repo and "DOCKER_PASSWORD" and "DOCKER_LOGIN" to log in to docker hub -- an API key with push permission would be an alternative: Could you create something similar to this for this GitHub workflow (and test it out with your own docker hub location / credentials)? Ideally, all we'd need to set is different ENV vars to push things to "openquantumsafe(/go)" after you already tested out the logic with(in) your own account. Also, the push should not happen on build but only when finally merging. Please see this example how we do it in oqs-demos: The code is CCI specific but you surely get the jist: The pattern is always the same: "build&test" on PR and "build, test & push" when merging to main branch.

ok, I will have a try with my own fork. and use github's docker registry service.

SamYuan1990 commented 1 year ago

https://github.com/SamYuan1990/liboqs-go/pkgs/container/liboqs-go here is the package and I tested following docker/build-push-action's guide

build pr tested via https://github.com/SamYuan1990/liboqs-go/pull/2

SamYuan1990 commented 1 year ago

I will further test those images tomorrow.

JannisFengler commented 1 year ago

For best practice, security, and reproducibility, I highly encourage using a linter like https://github.com/hadolint/hadolint .

baentsch commented 1 year ago

For best practice, security, and reproducibility, I highly encourage using a linter like https://github.com/hadolint/hadolint .

Interesting suggestion. I guess this would best be added to CI then, right? Would you be willing to make a contribution to facilitate this (maybe first for the current build)?

SamYuan1990 commented 1 year ago

I tested with my local fork, and the my local packages works on M1. @baentsch Hence I suppose the poc works well. Move further, let's do pr review and ask maintainers to decide to use github action or still on circleci(but seems need circleci ready for buildx?).

SamYuan1990 commented 1 year ago

any feedbacks for this PR are welcome, and I am going to refine this PR soon.

baentsch commented 1 year ago

any feedbacks for this PR are welcome, and I am going to refine this PR soon.

Looking forward to your update.

SamYuan1990 commented 1 year ago

here is the build result. https://github.com/SamYuan1990/liboqs-go/actions/runs/4392197575/jobs/7691827175

vsoftco commented 1 year ago

Thanks! I will review and merge those by next week

SamYuan1990 commented 1 year ago

Thanks! I will review and merge those by next week

Please be careful with repo specific settings in .github/workflows/PRcheck.yml , and please adjust it if needed after merge.

baentsch commented 1 year ago

Thanks! I will review and merge those by next week

ping @vsoftco . Or are you waiting for input from me? Just removed the "Change requested" mark as @SamYuan1990 has added CI as requested (Thanks!).

The one thing I leave to you to decide, @vsoftco is whether the location of the Dockerfile (project top dir) is OK for you.