Conversation
Signed-off-by: Haytham Abuelfutuh <[email protected]>
|
Bito Automatic Review Skipped - Branch Excluded |
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
🐳 Docker CI Image BuiltThe CI Docker image has been built and pushed for this PR! Image: This image will be automatically used by CI workflows in this PR. To test locally: docker pull ghcr.io/flyteorg/flyte/ci:pr-6671
docker run --rm -it -v $(pwd):/workspace -w /workspace ghcr.io/flyteorg/flyte/ci:pr-6671 bash |
Signed-off-by: Haytham Abuelfutuh <[email protected]>
|
Bito Automatic Review Skipped - Branch Excluded |
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
|
Bito Automatic Review Skipped - Branch Excluded |
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
|
Bito Automatic Review Skipped - Branch Excluded |
Signed-off-by: Haytham Abuelfutuh <[email protected]>
|
Bito Automatic Review Skipped - Branch Excluded |
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
|
Bito Automatic Review Skipped - Branch Excluded |
|
Bito Automatic Review Skipped - Branch Excluded |
Signed-off-by: Haytham Abuelfutuh <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces Docker-based development and CI workflows to ensure consistent environments across local development and CI. The changes standardize tool versions and eliminate "works on my machine" issues by containerizing the development environment.
Key changes:
- Creates a unified Docker image (
ci.Dockerfile) with pinned tool versions for Go, Python, Node.js, Rust, and Buf - Implements intelligent CI workflows that automatically build PR-specific Docker images when the Dockerfile is modified
- Updates the Makefile to support both Docker-based (default) and local tool-based development workflows
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/docker-dev.sh |
Helper script to simplify Docker container usage for development |
ci.Dockerfile |
Multi-stage Docker image with development tools and optimized caching |
Makefile |
Updated with Docker-based targets as defaults, local tool targets as alternatives |
.github/workflows/build-ci-image.yml |
Workflow to build and publish Docker images with PR-specific tagging |
.github/workflows/check-generate.yml |
Updated to use Docker containers and wait for PR-specific images |
docs/ |
Comprehensive documentation for Docker workflows and development environment |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]> Signed-off-by: Haytham Abuelfutuh <[email protected]>
|
Bito Automatic Review Skipped - Branch Excluded |
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
| gen: buf mocks go_tidy ## Generates everything in the 'gen' directory | ||
| @echo '⚡ Finished generating everything in the gen directory' | ||
| gen: buf mocks go-tidy ## Generate everything (uses Docker - no local tools required) | ||
| @echo '⚡ Finished generating everything in the gen directory (Docker)' |
There was a problem hiding this comment.
Can we use:
$(DOCKER_RUN) bash -c "git config --global --add safe.directory /workspace && make gen-local"Same as what we did in docker-gen-local? In this case we do not need buf-... and buf-...-local. We can just have buf-... as what we have originally
There was a problem hiding this comment.
I was keeping it so you can continue to run individual commands (but now using docker).... I'll follow your guidance and we can bring them back if needed.
Makefile
Outdated
|
|
||
| # Docker CI image configuration | ||
| DOCKER_CI_IMAGE := ghcr.io/flyteorg/flyte/ci:v2 | ||
| DOCKER_LOCAL_IMAGE := flyte-ci:local |
There was a problem hiding this comment.
Can we also name the image built locally as ghcr.io/flyteorg/flyte/ci:v2?
In this case, no matter using remote image or build locally, we can run make gen. If no changes in ci.Dockerfile, we use make docker-pull before make gen, otherwise use make docker-build.
The process will become:
- typical usage:
- pull CI image
make docker-pull - run
make gen
- If there's modification in
ci.Dockerfile:
- build the CI image
make docker-build - also run
make gen
I think it can make the process easier to follow and also make the Makefile cleaner
There was a problem hiding this comment.
Maybe we can use name gen.Dockerfile instead? As I think this is mainly for generating
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
docs/docker-image-workflow.md
Outdated
| │ check-generate workflow triggers │ | ||
| │ • Checks if Dockerfile.ci modified: NO │ |
There was a problem hiding this comment.
Documentation references Dockerfile.ci, but the repository adds gen.Dockerfile; update all occurrences to avoid confusion.
docs/docker-image-workflow.md
Outdated
|
|
||
| ### Image Detection Logic | ||
|
|
||
| Workflows check if `Dockerfile.ci` or `.github/workflows/build-ci-image.yml` were modified: |
There was a problem hiding this comment.
Example detection logic uses Dockerfile.ci instead of gen.Dockerfile; this is inconsistent with the actual file name and will mislead users.
| docker run --rm -it \ | ||
| -v "$REPO_ROOT:$WORKSPACE" \ | ||
| -w "$WORKSPACE" \ | ||
| "$IMAGE" \ | ||
| bash |
There was a problem hiding this comment.
[nitpick] Running as root in the container will create root-owned files on the host, complicating cleanup. Consider adding --user
| | Context | Image Tag | When Created | | ||
| |--------------------|----------------------------------------|---------------------------------| |
There was a problem hiding this comment.
Table rows start with double pipe characters, which can break Markdown table rendering. Remove the extra leading pipe on these lines.
Co-authored-by: Copilot <[email protected]> Signed-off-by: Haytham Abuelfutuh <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Haytham Abuelfutuh <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Haytham Abuelfutuh <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
|
Bito Automatic Review Skipped - Branch Excluded |
machichima
left a comment
There was a problem hiding this comment.
Not familiar with github action YAML, but tried running make docker-pull gen DOCKER_CI_IMAGE=ghcr.io/flyteorg/flyte/ci:pr-6671 locally and it works.
Overall LGTM! Just some small nits on docs
Co-authored-by: Nary Yeh <[email protected]> Signed-off-by: Haytham Abuelfutuh <[email protected]>
Co-authored-by: Nary Yeh <[email protected]> Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
|
Bito Automatic Review Skipped - Branch Excluded |
machichima
left a comment
There was a problem hiding this comment.
Sorry I miss this. I think it should be master instead of main branch
| on: | ||
| push: | ||
| branches: | ||
| - main |
There was a problem hiding this comment.
| - main | |
| - master |
There was a problem hiding this comment.
So I created a main branch.. it's where I deleted everything from the master branch... I think what we will do is when we are ready to switch over, we will switch the default branch for the repo from master to main... and let master be the v1 stuff
| # Use PR number for pull requests | ||
| type=ref,event=pr | ||
| # Use 'latest' tag for main branch | ||
| type=raw,value=latest,enable=${{ github.ref == 'refs/heads/main' }} |
There was a problem hiding this comment.
| type=raw,value=latest,enable=${{ github.ref == 'refs/heads/main' }} | |
| type=raw,value=latest,enable=${{ github.ref == 'refs/heads/master' }} |
| echo "tag=pr-${{ github.event.pull_request.number }}" >> $GITHUB_OUTPUT | ||
| elif [ "${{ github.ref }}" == "refs/heads/v2" ]; then | ||
| echo "tag=v2" >> $GITHUB_OUTPUT | ||
| elif [ "${{ github.ref }}" == "refs/heads/main" ]; then |
There was a problem hiding this comment.
| elif [ "${{ github.ref }}" == "refs/heads/main" ]; then | |
| elif [ "${{ github.ref }}" == "refs/heads/master" ]; then |
| type=ref,event=branch | ||
| # Use PR number for pull requests | ||
| type=ref,event=pr | ||
| # Use 'latest' tag for main branch |
There was a problem hiding this comment.
| # Use 'latest' tag for main branch | |
| # Use 'latest' tag for master branch |
|
|
||
| 1. **`.github/workflows/build-ci-image.yml`** | ||
| - Builds Docker image | ||
| - Runs on: PR with Dockerfile changes, push to main/v2 |
There was a problem hiding this comment.
| - Runs on: PR with Dockerfile changes, push to main/v2 | |
| - Runs on: PR with Dockerfile changes, push to master/v2 |
| | Regular PR | `v2` or `latest` | Uses existing branch image | | ||
| | PR with Dockerfile | `pr-123` | Built when PR is created/updated| | ||
| | v2 branch push | `v2` | Built on every push to v2 | | ||
| | main branch push | `latest` | Built on every push to master | |
There was a problem hiding this comment.
| | main branch push | `latest` | Built on every push to master | | |
| | master branch push | `latest` | Built on every push to master | |
Signed-off-by: Haytham Abuelfutuh [email protected]
Tracking issue
Why are the changes needed?
What changes were proposed in this pull request?
How was this patch tested?
Labels
Please add one or more of the following labels to categorize your PR:
This is important to improve the readability of release notes.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link