-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add workflow to master branch #476
base: ros2
Are you sure you want to change the base?
Conversation
WalkthroughA new GitHub Actions workflow Changes
Sequence DiagramsequenceDiagram
participant Trigger as Workflow Trigger
participant Matrix as Build Matrix
participant Docker as Docker Image Build
participant Registry as Docker Hub
Trigger->>Matrix: Invoke with parameters
Matrix->>Docker: Build ARM64 Image
Matrix->>Docker: Build AMD64 Image
Docker->>Registry: Push Images
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/build-docker.yaml (3)
27-51
: Consider reusing input definitions to avoid duplication.The input definitions are duplicated between
workflow_call
andworkflow_dispatch
. Consider extracting common configurations to reusable YAML anchors to maintain consistency and reduce duplication.Example using YAML anchors:
# At the top of the file definitions: &common_inputs target_distro: &target_distro description: In case of "stable" release specify the ROS distro of the existing docker image (eg. humble) type: string default: humble target_release: &target_release description: In case of "stable" release specify the version of the existing docker image (eg. 1.0.12) type: string default: 0.0.0 target_date: &target_date description: In case of "stable" release specify the date of the existing docker image in format YYYYMMDD (eg. 20220124) type: string default: '20131206' on: workflow_call: inputs: build_type: description: Is it a "development" or a "stable" release? type: string default: development target_distro: <<: *target_distro target_release: <<: *target_release target_date: <<: *target_date workflow_dispatch: inputs: build_type: description: Is it a "development" or a "stable" release? type: choice options: [development, stable] default: development target_distro: <<: *target_distro target_release: <<: *target_release target_date: <<: *target_date
76-76
: Consider making main_branch_name configurable.The
main_branch_name
is hardcoded to 'ros2'. Consider making it configurable through workflow inputs if different branches might be used in the future.
82-85
: Improve comment clarity for stable release variables.The comment could be more specific about when and how these variables are used. Consider updating it to explain their purpose and impact on the build process.
- # variables important only for stable release + # These variables are used when build_type is "stable" to tag the image correctly
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-docker.yaml
(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/build-docker.yaml
11-11: input "build_type" of workflow_call event has the default value "development", but it is also required. if an input is marked as required, its default value will never be used
(events)
🔇 Additional comments (3)
.github/workflows/build-docker.yaml (3)
1-3
: LGTM!The workflow name is clear and descriptive.
22-26
: Verify the default target_date value.The default target_date is set to '20131206' which seems outdated. Consider updating it to a more recent date or removing the default value if it's not needed.
58-65
: Verify repo_name configuration in matrix.The
repo_name
is only defined for the simulation build. Is this intentional? If not, consider adding it to the hardware build configuration as well.
build_type: | ||
description: Is it a "development" or a "stable" release? | ||
required: true | ||
type: string | ||
default: development |
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 redundant required flag or default value for build_type input.
The input is marked as required but also has a default value. This configuration is contradictory since the default value will never be used for required inputs.
Choose one of these options:
build_type:
description: Is it a "development" or a "stable" release?
- required: true
type: string
default: development
OR
build_type:
description: Is it a "development" or a "stable" release?
required: true
type: string
- default: development
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
build_type: | |
description: Is it a "development" or a "stable" release? | |
required: true | |
type: string | |
default: development | |
build_type: | |
description: Is it a "development" or a "stable" release? | |
type: string | |
default: development |
🧰 Tools
🪛 actionlint (1.7.4)
11-11: input "build_type" of workflow_call event has the default value "development", but it is also required. if an input is marked as required, its default value will never be used
(events)
Description
Requirements
Summary by CodeRabbit
New Features
Chores