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

Build in Github CI #41

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

Build in Github CI #41

wants to merge 9 commits into from

Conversation

majst01
Copy link
Collaborator

@majst01 majst01 commented Apr 26, 2024

This will close several bugs and CVEs by:

  • updating the golang.org/x/* dependencies to their actual released versions
  • update google.golang.org/protobuf and google.golang.org/grpc with their latest versions

Get rid of the vendor directory which is by now a good practice even in big and important projects like kubernetes

Build and publish the container image in the github actions and push it to ghcr.io

@muliby-lb
Copy link
Collaborator

@rahman-lb can you and the team please take care of this for both los-csi and lb-csi
CC @ronen-lb

@muliby-lb muliby-lb assigned muliby-lb and rahman-lb and unassigned muliby-lb and rahman-lb Apr 30, 2024
@rahman-lb
Copy link
Collaborator

@majst01 can you add a few lines of description to the PR ?

@rahman-lb rahman-lb self-requested a review May 1, 2024 04:12
@rahman-lb
Copy link
Collaborator

I'm testing this change on internal repo, will merge after passing the tests.

@majst01
Copy link
Collaborator Author

majst01 commented May 1, 2024

What was the output of the failing test ?

@majst01
Copy link
Collaborator Author

majst01 commented May 1, 2024

@majst01 can you add a few lines of description to the PR ?

done, hope this helps

rahman-lb
rahman-lb previously approved these changes May 2, 2024
@muliby-lb
Copy link
Collaborator

@rahman-lb pls merge both here and in lb-csi. Also, you may want to prepare the plugin for the next release (CC @ronen-lb) now to avoid having to do it in the last few days before 3.9.1.

@rahman-lb
Copy link
Collaborator

@majst01 can you check why PR check is failing ?

@majst01
Copy link
Collaborator Author

majst01 commented May 5, 2024

@majst01 can you check why PR check is failing ?

The logs from the build in the CI are not visible here, at least for me, which makes it hard to tell, can you send me the logs over ?

@majst01
Copy link
Collaborator Author

majst01 commented May 6, 2024

BTW: any reason why not building in github actions and publish the container to the github container registry for every PR ?

@rahman-lb
Copy link
Collaborator

That will be the right approach, eliminate outside dependencies and let this repo hold everything needed including build artifacts.

@majst01
Copy link
Collaborator Author

majst01 commented May 7, 2024

That will be the right approach, eliminate outside dependencies and let this repo hold everything needed including build artifacts.

I can provide a separate PR to build on github actions and publish to ghcr.io if you are willing to merge.

@majst01
Copy link
Collaborator Author

majst01 commented May 7, 2024

Now added the build and push of the container to ghcr.io in the github actions, but to be able to pull the container without credentials, someone with repo admin rights must enable anonymous pulls here:

https://github.com/LightBitsLabs/los-csi/pkgs/container/los-csi

@rahman-lb
Copy link
Collaborator

rahman-lb commented May 8, 2024

@majst01 I think you have access to upload packages using your PAT token, try to download the already published image , if you run into permission then i can allow anonymous download or give you permission for read/write.

Unless someone has any objection, it seems to me that publishing images to ghcr.io is the right approach. I'll be happy to merge Gitub actions into main.

@ronen-lb @yogev-lb

@majst01
Copy link
Collaborator Author

majst01 commented May 8, 2024

@majst01 I think you have access to upload packages using your PAT token, try to download the already published image , if you run into permission then i can allow anonymous download or give you permission for read/write.

Sure i know this, but to be able to take the container image pushed from a PR, it would be nice to be able to test this in our integration tests without adding a pull secret which is not required for the released images.

Unless someone has any objection, it seems to me that publishing images to ghcr.io is the right approach. I'll be happy to merge Gitub actions into main.

@ronen-lb @yogev-lb

WORKDIR /work
COPY . .

RUN make build
Copy link
Collaborator

Choose a reason for hiding this comment

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

@majst01 I'm not sure, how this change worked for you becuase you're copying the content of the /deploy folder into the container which doesn't include the source code and make file but you're trying to build binary which requires the source code and make file. Ideally, we want to keep the binary build up outside the Dockerfile. This is the reason for the current CI failure, i removed this change from this PR #44 and CI jobs are passing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The COPY . . instruction copies the whole directory into the container. The Build succeeded in the github CI as you can see here in the pipeline. As i dont know how your CI works and the logs are not visible i can not tell why this failed.

Building binaries inside a multistage container build is best practice, because you do not relay on the CI infrastructure which is unknown for contributors and might change.

@majst01 majst01 changed the title Update most direct go dependencies Build in Github CI May 21, 2024
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot

See analysis details on SonarCloud

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