Conversation
|
The CI won't run until I merge this PR. Might have to make a few additional fixes. EDIT: I pushed a simple ci.yml to main to so the new ci will run now. |
There was a problem hiding this comment.
Pull request overview
This PR adds Docker images for Ice 3.8 services and migrates from a Makefile-based build system to GitHub Actions CI/CD. The changes introduce four new Ice 3.8 service images (glacier2, icegridnode, icegridregistry, icestorm) using Ubuntu 24.04 as the base image.
Key Changes:
- Added complete 3.8 Docker images with Dockerfiles and entrypoint scripts for all four Ice services
- Replaced manual Makefile build system with automated GitHub Actions workflow for building and pushing images
- Removed outdated 3.7 README documentation files
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Removed old manual build system for 3.7 images |
| .github/workflows/ci.yml | New CI workflow for building and pushing Docker images across versions |
| 3.8/glacier2/Dockerfile | Dockerfile for Glacier2 router service on Ubuntu 24.04 |
| 3.8/glacier2/docker-entrypoint.sh | Entrypoint script for Glacier2 service |
| 3.8/icegridnode/Dockerfile | Dockerfile for IceGrid node service on Ubuntu 24.04 |
| 3.8/icegridnode/docker-entrypoint.sh | Entrypoint script for IceGrid node service |
| 3.8/icegridregistry/Dockerfile | Dockerfile for IceGrid registry service on Ubuntu 24.04 |
| 3.8/icegridregistry/docker-entrypoint.sh | Entrypoint script for IceGrid registry service |
| 3.8/icestorm/Dockerfile | Dockerfile for IceStorm service on Ubuntu 24.04 |
| 3.8/icestorm/docker-entrypoint.sh | Entrypoint script for IceStorm service |
| 3.7/glacier2/README.md | Removed outdated 3.7 documentation |
| 3.7/icegridnode/README.md | Removed outdated 3.7 documentation |
| 3.7/icegridregistry/README.md | Removed outdated 3.7 documentation |
| 3.7/icestorm/README.md | Removed outdated 3.7 documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pepone
left a comment
pepone
left a comment
There was a problem hiding this comment.
Looks good. This PR removed all READMEs, don't we need a README for the images?
|
|
||
| COPY ./docker-entrypoint.sh / | ||
| ENTRYPOINT ["/docker-entrypoint.sh"] | ||
| CMD ["glacier2router", "--Ice.Config=/etc/glacier2router.conf"] |
There was a problem hiding this comment.
Why do we need glacier2router and --Ice.Config here and in the entry point?
There was a problem hiding this comment.
I believe the original intention (that I borrowed from some other container) is so that the cmd can be overwritten by the user easily.
| && sed -i 's/^IceGrid\.Registry\.Server\.Endpoints=.*/IceGrid\.Registry\.Server\.Endpoints=tcp -h 0\.0\.0\.0/' /etc/icegridregistry.conf \ | ||
| && sed -i 's/^IceGrid\.Registry\.Internal\.Endpoints=.*/IceGrid\.Registry\.Internal\.Endpoints=tcp -h 0\.0\.0\.0/' /etc/icegridregistry.conf | ||
|
|
||
| EXPOSE 4061 4062 |
There was a problem hiding this comment.
Do we need both, seems we only listen on 4061?
| && sed -i 's/^Ice.Default.Locator=.*/#Ice.Default.Locator=/' /etc/icegridnode.conf \ | ||
| && sed -i 's/IceGrid.Node.Endpoints=.*/IceGrid.Node.Endpoints=tcp -h 0\.0\.0\.0/' /etc/icegridnode.conf | ||
|
|
||
| EXPOSE 4061 4062 |
| COPY ./docker-entrypoint.sh / | ||
| ENTRYPOINT ["/docker-entrypoint.sh"] | ||
|
|
||
| CMD ["icebox", "--IceBox.Service.IceStorm=IceStormService,38:createIceStorm", "--IceStorm.LMDB.Path=/data"] |
There was a problem hiding this comment.
We should consider adding dsnode image for 3.8
There was a problem hiding this comment.
I'll add in a follow-up PR.
I need to figure out where to put them. Does make sense to have them in the version dirs. It used to be that docker would automatically upload them when it used to do its own image building, but that's no longer the case. Maybe I'll make top level service dirs with the README |
|
I've updated the READMEs and moved them into a docs folder. |
No description provided.