-
Notifications
You must be signed in to change notification settings - Fork 2
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
Enable release image building #104
Conversation
Fix indentation issue.
images: ghcr.io/nwchemex-project/release_${{needs.Repo_name_lowcase.outputs.lowcase_repo_name}} | ||
- name: Build the image and push | ||
if: ${{inputs.gcc-build == true}} | ||
uses: docker/build-push-action@v4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this going to push every time this workflow is run? Isn't this also the workflow used for testing the PRs? We don't want those to be the same workflow as the deployment is different than the testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this going to push every time this workflow is run? Isn't this also the workflow used for testing the PRs? We don't want those to be the same workflow as the deployment is different than the testing.
So you mean a separated workflow simply to deploy the code, right? What would be the event to trigger this workflow, say a successful testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's some sketches meant to depict how I'm thinking about the various use cases. The sketch shows the DockerFiles living in the respective repos. If you're able to create a single DockerFile which can be used to build each repo's base image (presumably by using variables for the the base image's base image and the dependencies to build) and/or a single DockerFile for building the release images (presumably by using variables for the base image) then those common DockerFiles can live here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this going to push every time this workflow is run? Isn't this also the workflow used for testing the PRs? We don't want those to be the same workflow as the deployment is different than the testing.
Added options to install the package, build and push the release docker image only when necessary. Different calls would come from different workflow in the corresponding repo invoked by different events, e. g., PR or push to master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, this is going to be a huge change to our CI. We should probably setup a dummy organization and iron out all the kinks in an end-to-end solution before pushing them to the NWX org.
I agree. How to copy the entire organization to a new one? |
I don't think you need to copy the whole org, just .github, ParallelZone, and PluginPlay. Once you have those going that should be 99% of the battle. |
Ok, I think I have already created an organization for CI testing, just trying to put my forked branches of .github, ParallelZone and PluginPlay into it. |
You should be able to fork the repos as the organization you're trying to put them in. |
Got it. Now all 3 repos are in my CI-test organization. |
Yu Zhang seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being blunt, I feel like this PR is part of a prototyping effort, which if that's the case I don't want to merge it. Keep in mind our CI/CD pipeline affects the entire team and we don't want to be using it for prototyping. PRs targeting CI/CD should be moving us towards a sustainable solution, and not contributing to flux. We've admittedly not always been the best at this, but now that we've actually got some software releases I think we need to do better. My hope is that we can do one last overhaul and achieve a stable CI/CD pipeline.
To that end, based on my recollection of past discussions with the CI team, I put together #105 to try and articulate the complexities our CI/CD solution needs to address. I have a design in my head which I think will address them all; however, it relies on a more fine-grained approach to workflows and images than where this seems to be heading. Of course, just because you're not using my design, doesn't mean it won't work, but if we're going to go with a different design there needs to be at least a couple people that understand that design in order to review PRs associated with it.
Long story short, I'm fine with whatever solution we put together that will address the issues in #105, but until we have that solution, or a path to that solution, I think we're better off not merging anything.
So what I propose:
- Fully sketch out a CI/CD pipeline design, or
- Get CI/CD working for a copy of the NWChemEx-Project organization and then port CI/CD form the copy to the real project.
I think that's a start, but there's more complexity in play when you get further along. Notably the dependency diamond problem (package A is used by packages B and C; package D then depends on packages B and C and thus depends on A through both B and C). |
PR Type
Brief Description
Enable installating of a repo after its successful building, then build a docker image for the installed library and push it to the nwchemex-project registry.
Not In Scope
PR Checklist
TODOs