open-quantum-safe / ci-containers

CI images for testing OQS projects.
https://openquantumsafe.org/
MIT License
4 stars 8 forks source link

update go to 1.22.2 #77

Closed pi-314159 closed 6 months ago

pi-314159 commented 6 months ago

Related to https://github.com/open-quantum-safe/boringssl/pull/115

A better way is to create an image for ubuntu noble...

pi-314159 commented 6 months ago

@SWilson4 @praveksharma could you take a look at this? I believe BoringSSL is the only OQS project that uses Go.

SWilson4 commented 6 months ago

@SWilson4 @praveksharma could you take a look at this? I believe BoringSSL is the only OQS project that uses Go.

There's also liboqs-go, but it doesn't use the ci-containers for CI, and it requires Go >= 1.21 anyhow.

Please do feel free to add a Noble Dockerfile if you feel that it would be useful for maintaining BoringSSL.

SWilson4 commented 6 months ago

I am curious as to why CI is failing: @praveksharma you had a recent PR for which everything was green, any ideas on what could be going wrong? (Not suggesting that you broke it, just wondering if you know how to fix it)

pi-314159 commented 6 months ago

There's also liboqs-go, but it doesn't use the ci-containers for CI, and it requires Go >= 1.21 anyhow.

Ehh yes; I mean updating Go here shouldn't affect other OQS projects. I'll create a Noble Dockerfile when I have time.

CI fails at "docker login"; I guess it's because I don't have permission to use ci.

baentsch commented 6 months ago

any ideas on what could be going wrong?

@pi-314159 has it right:

I don't have permission

(to access the credentials to push the image to the open-quantum-safe docker hub account).

So two ways forward:

1) Provide @pi-314159 with the proper permissions 2) Create a split CI path: No docker login during PR and docker login && push only during merge (if possible: There was a reason why we didn't implement it that way at the start but I can't remember any more).

pi-314159 commented 6 months ago

I'm confident that this update won't cause any problem. Can we merge it first?

baentsch commented 6 months ago

I'm confident that this update won't cause any problem. Can we merge it first?

I tend to agree -- but why do we have CI if we don't use it? Just tried to move this into an org setup that it can successfully pass first... Now all CI should run and the images if OK automatically getting deployed.

@SWilson4 @praveksharma Please provide other solutions as you see fit to move this forward.

SWilson4 commented 6 months ago

I'm confident that this update won't cause any problem. Can we merge it first?

I'm OK with that if it will unblock you.

So two ways forward:

1. Provide @pi-314159 with the proper permissions

2. Create a split CI path: No `docker login` during PR and `docker login && push` only during merge (if possible: There was a reason why we didn't implement it that way at the start but I can't remember any more).

I prefer (1); I'd rather not find out the reason we didn't do (2) by painful experience. :slightly_smiling_face:

praveksharma commented 6 months ago

I prefer (1); I'd rather not find out the reason we didn't do (2) by painful experience. 🙂

I agree with this, since it seems more straightforward to implement and also because we will probably transitioning to GitHub Actions once it supports ARM.

baentsch commented 6 months ago

Now all CI should run and the images if OK automatically getting deployed.

This just happened. All it took was a git remote add & checkout & push cascade. Merging. Thanks for the enhancement, @pi-314159 .

baentsch commented 6 months ago

we will probably transitioning to GitHub Actions once it supports ARM.

What's the connection between GH actions and having CI containers containing a suitable build/test env?

praveksharma commented 6 months ago

What's the connection between GH actions and having CI containers containing a suitable build/test env?

My understanding is that we aren't running the "build" check on GH actions because we need ARM runners on CirlceCI. (2) seems like a more suitable long-term solution to more easily accept PRs from new/external contributors.

I prefer (1); I'd rather not find out the reason we didn't do (2) by painful experience. 🙂

I agree with this, since it seems more straightforward to implement and also because we will probably transitioning to GitHub Actions once it supports ARM.

What I meant to say: putting in the effort to make (2) work (correctly) doesn't seem to make sense since we may soon switch to GH actions once ARM runners are available, so go ahead with (1) instead.

baentsch commented 6 months ago

My understanding is that we aren't running the "build" check on GH actions because we need ARM runners on CirlceCI.

That understanding is also mine. We can dump CCI the moment we have GH ARM runners (any news where they are?). But we still need ARM docker images in that case, right? But then again, if your statement was simply to say that (1) is easiest and a quick short-term fix, agreed. But we now used (3) successfully: git remote add & checkout & push. So case closed.