search

open-quantum-safe / liboqs-go

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

fixup for #21 #22

Closed baentsch closed 1 year ago

baentsch commented 1 year ago

Attempt fixing #21.

baentsch commented 1 year ago

@SamYuan1990 Please check the (remaining ) error message: There seems to be something fishy with the build-image instruction added in #20: https://github.com/open-quantum-safe/liboqs-go/actions/runs/4685702520/jobs/8303055705?pr=22

SamYuan1990 commented 1 year ago

@SamYuan1990 Please check the (remaining ) error message: There seems to be something fishy with the build-image instruction added in #20: https://github.com/open-quantum-safe/liboqs-go/actions/runs/4685702520/jobs/8303055705?pr=22

you know as I said @baentsch , please ask who has maintainer role to fix it as https://github.com/open-quantum-safe/liboqs-go/blob/main/.github/workflows/PRcheck.yml#L29 in this PR it requires environment vars. and share with my fork settings.

image
SamYuan1990 commented 1 year ago

hi/hello @baentsch ，any feedback?

baentsch commented 1 year ago

hi/hello @baentsch ，any feedback?

@vsoftco , do you have such maintainer rights (See comment above)?

SamYuan1990 commented 1 year ago

vars.DOCKERIMAGE needs to be a variable.

for this, @xvzcf , for contributor. as he/she if not maintainer for this repo. which means he/she can't push package to this repo but to he/she own fork. and when the PR been merged, the upstream action can't not push to fork repo's package either. or in opposite direction from personal fork to upstream main, which will be a breaking change as general case for any CI related changes.

Hence here, by a variable, we can targeting the repo to either fork or upstream by just setting up in settings without a breaking change. (Same for secret to access docker repo or github docker registration)

Obviously, if PR's contributor without maintain role to contribute CI related PR, this things may happen.

xvzcf commented 1 year ago

Understood, but with a variable, the name of the Docker image is being obscured in the push step, and I'm not convinced that fixing the mismatch between upstream and forks is worth this.

That being said, I won't block on this.

SamYuan1990 commented 1 year ago

as I don't have permission to edit CI related env or secret for this repo, I try to provide a fix as #23 for reference. @baentsch , @xvzcf sorry for replay late.

baentsch commented 1 year ago

Understood, but with a variable, the name of the Docker image is being obscured in the push step, and I'm not convinced that fixing the mismatch between upstream and forks is worth this.

This is not the problem, @xvzcf but rather that the location of the docker image changed in #20; in addition, CircleCI doesn't seem to offer docker buildx support, which is why this build-and-push test got moved to github CI. Thus, removal of the CCI code to build the docker image using "buildx" is sensible (as quickly done in this pre-vacation hotfix) and addition of the required secrets to github CI (as requested by @SamYuan1990 ).

the image building and pushing should only occur in the main branch, when (presumably) all the other checks have passed.

I do think it does as shown by this code, no?:

https://github.com/open-quantum-safe/liboqs-go/blob/28be2ce91ace968370f3f04b38d2128b160396c0/.github/workflows/build.yml#L44-L48

One may argue that it would be more sensible to set the value of the push argument below not to "true" but to "github.ref == 'refs/heads/main'" thus allowing build but no push on a branch other than "main".

vsoftco commented 1 year ago

Decided to drop support for this, as it introduce additional dependencies that are difficult to maintain in the long term.