-
Notifications
You must be signed in to change notification settings - Fork 249
docs: Update README with Docker image generation documentation #3723
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: jpayne3506 <[email protected]>
@copilot combine make recipes documentation to be generic |
Co-authored-by: jpayne3506 <[email protected]>
Made the documentation generic with a standardized pattern that applies to all components. The documentation now uses |
@copilot move these changes to ./doc/README.md |
Co-authored-by: jpayne3506 <[email protected]>
@copilot I meant docs/README.md |
…sted Co-authored-by: jpayne3506 <[email protected]>
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.
Pull Request Overview
This PR adds comprehensive documentation for Docker image generation in the repository, detailing component-specific build commands, prerequisites, and customization options.
- Added a new "Docker Image Generation" section to docs/README.md covering prerequisites, available components, generic build patterns, example usage, and customization.
- Updated the top-level README.md to reference the new Docker documentation.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
docs/README.md | New section documenting Docker image generation, build patterns, and component-specific commands for networking. |
README.md | Added a link reference to the Docker image generation documentation in docs/README.md. |
Comments suppressed due to low confidence (1)
docs/README.md:22
- The 'Available Components' section lists 'acncli' and 'ipv6-hp-bpf', but the PR description mentions only four core components. Consider clarifying whether these additional components are part of the core set or updating the documentation to match the intended scope.
+- **acncli** - CNI manager
/azp run Azure Container Networking PR |
Azure Pipelines successfully started running 1 pipeline(s). |
@copilot, document how GOOS=windows impacts image generation |
Co-authored-by: jpayne3506 <[email protected]>
Co-authored-by: jpayne3506 <[email protected]>
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.
Thanks for updating the readme @jpayne3506
/azp run Azure Container Networking PR |
Azure Pipelines successfully started running 1 pipeline(s). |
docs/README.md
Outdated
- `PLATFORM` - Target platform/architecture (default: linux/amd64) | ||
- `IMAGE_REGISTRY` - Custom registry (default: acnpublic.azurecr.io) | ||
- `CONTAINER_BUILDER` - Container builder (default: docker, alternative: podman) | ||
- `GOOS` - Target operating system for cross-compilation (default: linux) |
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.
remove - GOOS
is extracted by parsing PLATFORM
and should not be set separately
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.
@copilot continue removing documentation of GOOS
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.
Removed all GOOS documentation from docs/README.md as requested. The build system automatically extracts GOOS from the PLATFORM variable, so users only need to specify PLATFORM. See commit f0767b3.
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.
I interpreted
azure-container-networking/Makefile
Line 225 in 4aa36ae
OS ?= $(GOOS) |
GOOS
to change OS
. Should we be instead manually overwriting OS
?
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.
well, i know i wrote code to parse the platform in to OS/ARCH somewhere, at some point. but you're right, that's not what's happening here now. i think especially for the container builds we should do it.
maybe we default platform as PLATFORM ?= $GOOS/$GOARCH
to pick up the host platfom. but if it is set then we should parse $OS and $ARCH out of it, and then set any go
invocation GOOS and GOARCH to those.
Co-authored-by: rbtr <[email protected]>
Co-authored-by: rbtr <[email protected]>
Added comprehensive Docker image generation documentation to the main README covering all four core networking components as requested in the issue.
Changes Made
Added a new "Docker Image Generation" section to README.md that includes:
Prerequisites
Component-Specific Build Commands
Documentation for building Docker images for each of the four requested components:
Advanced Features
Example Usage
The documentation follows the existing repository style and provides clear, actionable instructions for developers who need to build container images for these components.
Fixes #3722.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.