feat: add support for build arguments when building Docker images usi… #569
Conversation
WalkthroughThe changes introduce support for build arguments when building Docker images using Kaniko. A new test function is added to validate this functionality, and several modifications are made across various files to implement a structured approach for handling build arguments. This includes updates to methods for setting images and Git repositories, as well as the introduction of new types and interfaces for managing these arguments. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
pkg/builder/args.go (2)
10-23: LGTM: BuildArg struct and methods are well-implemented.The
BuildArgstruct and its methods correctly implement theArgInterface. The use of a type assertionvar _ ArgInterface = &BuildArg{}is a good practice to ensure the interface is properly implemented at compile-time.Consider adding a brief comment explaining the purpose of the type assertion:
// Ensure BuildArg implements ArgInterface var _ ArgInterface = &BuildArg{}
1-39: Overall implementation is solid with room for minor improvements.The
args.gofile introduces a well-structured approach to handling build arguments, aligning closely with the PR objectives. The use of an interface (ArgInterface) and two implementing types (BuildArgandCustomArg) provides flexibility and extensibility.Consider the following suggestions to further improve the implementation:
- Add package-level documentation explaining the purpose and usage of this file.
- Include examples in the documentation for both
BuildArgandCustomArgto illustrate their intended use cases.- If
CustomArgis intended for future extensibility, consider adding a comment explaining this rationale.Example package-level documentation:
// Package builder provides functionality for handling build arguments // when building Docker images using Kaniko. It defines interfaces and // types for managing both standard build arguments and custom arguments.These improvements would enhance the code's readability and maintainability.
e2e/system/build_image_test.go (2)
95-134: LGTM: New test function effectively verifies build argument support.The
TestBuildWithBuildArgsfunction is well-structured and aligns with the PR objectives. It effectively tests the new functionality of passing build arguments when building Docker images using Kaniko.A minor suggestion for improvement:
Consider adding a comment explaining the purpose of
strings.TrimSpaceon line 131. This will help future maintainers understand why this step is necessary. For example:// Trim whitespace to ensure consistent comparison gotData = []byte(strings.TrimSpace(string(gotData)))
Removing the start command impacts the test's functionality.
The
TestBuildFromGitWithModificationsfunction relies onSetStartCommandto define how the target instance starts. Since there is no Dockerfile settingENTRYPOINTorCMD, removing this line will prevent the instance from starting correctly, potentially causing the test to fail or behave unexpectedly.🔗 Analysis chain
Line range hint
54-93: Verify the impact of removing the start command.The removal of the line setting the start command for the target instance simplifies the test setup. However, please ensure that this change doesn't affect the test's effectiveness or introduce any potential issues.
To verify the impact, please run the following script:
To ensure the removal of the start command doesn't negatively impact the test, please run the following scripts:
To further verify the impact of removing the start command, please execute the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the start command is set elsewhere or if it's necessary for the test. # Test 1: Search for any other occurrences of setting the start command echo "Searching for other occurrences of setting the start command:" rg --type go 'SetStartCommand|StartCommand' e2e/ # Test 2: Check if the Dockerfile in the test branch sets an entrypoint or CMD echo "Checking Dockerfile in the test branch for entrypoint or CMD:" git show test/build-from-git:Dockerfile | grep -E '(ENTRYPOINT|CMD)'Length of output: 1731
Script:
#!/bin/bash # Description: Verify the existence of the 'test/build-from-git' branch and locate Dockerfiles. # Test 1: List all branches to confirm 'test/build-from-git' exists echo "Listing all branches to verify 'test/build-from-git' exists:" git branch -a # Test 2: If 'test/build-from-git' exists, search for Dockerfiles in that branch if git show-ref --verify --quiet refs/heads/test/build-from-git; then echo "Searching for Dockerfiles in the 'test/build-from-git' branch:" git ls-tree -r test/build-from-git --name-only | grep -i 'Dockerfile' else echo "Branch 'test/build-from-git' does not exist." fiLength of output: 403
Script:
#!/bin/bash # Description: Locate the Dockerfile in the 'main' branch and check for ENTRYPOINT or CMD directives. # Test 1: Search for Dockerfiles in the 'main' branch echo "Searching for Dockerfiles in the 'main' branch:" git ls-tree -r main --name-only | grep -i 'Dockerfile' # Test 2: If Dockerfile is found, check for ENTRYPOINT or CMD DOCKERFILE=$(git ls-tree -r main --name-only | grep -i 'Dockerfile' | head -n 1) if [ -n "$DOCKERFILE" ]; then echo "Checking '$DOCKERFILE' for ENTRYPOINT or CMD directives:" git show main:"$DOCKERFILE" | grep -E '(ENTRYPOINT|CMD)' else echo "No Dockerfile found in the 'main' branch." fiLength of output: 364
pkg/builder/kaniko/kaniko_test.go (1)
49-49: LGTM! Consider enhancing the test case.The change from a slice of strings to a slice of
builder.ArgInterfacefor theArgsfield aligns well with the PR objectives to add support for build arguments. This new structure provides more flexibility in handling build arguments.To further improve the test coverage, consider enhancing the test case to verify that the build argument is correctly passed to and used by the Kaniko builder. This could involve:
- Modifying the
completeAllJobInFakeClientsetfunction to capture the Job specification.- Adding an assertion to check if the build argument is present in the Job's container arguments.
Here's a sample implementation:
func completeAllJobInFakeClientset(t *testing.T, clientset *fake.Clientset, namespace string) *batchv1.Job { ctx := context.Background() jobs, err := clientset.BatchV1().Jobs(namespace).List(ctx, metav1.ListOptions{}) assert.NoError(t, err) var capturedJob *batchv1.Job for _, j := range jobs.Items { j.Status.Succeeded = 1 updatedJob, err := clientset.BatchV1().Jobs(namespace).Update(ctx, &j, metav1.UpdateOptions{}) require.NoError(t, err) capturedJob = updatedJob // Create a Pod with the same name as the Job pod := createPodFromJob(&j) _, err = clientset.CoreV1().Pods(namespace).Create(ctx, pod, metav1.CreateOptions{}) require.NoError(t, err) } return capturedJob } // In the test case: capturedJob := completeAllJobInFakeClientset(t, k8sCS, k8sNamespace) assert.Contains(t, capturedJob.Spec.Template.Spec.Containers[0].Args, "--build-arg=SOME_ARG=some_value", "Build argument should be passed to the Kaniko container")This enhancement would provide stronger assurance that the new build argument functionality is working as expected.
pkg/container/docker.go (2)
101-101: LGTM with a minor suggestion: UpdatedPushBuilderImageto use build argumentsThe addition of
Args: f.argsto theBuilderOptionscorrectly passes the build arguments to the underlying builder. This change aligns with the PR objectives.Consider adding a nil check before setting the
Argsfield to handle cases where no build arguments are provided:if len(f.args) > 0 { Args: f.args, }This would prevent passing an empty slice of arguments when none are specified.
139-139: LGTM with a minor suggestion: UpdatedBuildImageFromGitRepoto use build argumentsThe addition of
Args: f.argsto theBuilderOptionscorrectly passes the build arguments to the underlying builder when building an image from a Git repository. This change aligns with the PR objectives.Similar to the suggestion for
PushBuilderImage, consider adding a nil check:if len(f.args) > 0 { Args: f.args, }This would prevent passing an empty slice of arguments when none are specified.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- e2e/system/build_image_test.go (2 hunks)
- pkg/builder/args.go (1 hunks)
- pkg/builder/builder.go (1 hunks)
- pkg/builder/kaniko/kaniko.go (3 hunks)
- pkg/builder/kaniko/kaniko_test.go (1 hunks)
- pkg/container/docker.go (4 hunks)
- pkg/instance/build.go (4 hunks)
🔇 Additional comments (17)
pkg/builder/args.go (3)
1-3: LGTM: Package declaration and constant definition are appropriate.The package name
builderis suitable for the functionality being implemented. The constantbuildArgKeywith the value "--build-arg" aligns well with the PR objectives of supporting build arguments for Docker image building using Kaniko.
5-8: LGTM: Well-designed interface for argument handling.The
ArgInterfaceis well-defined with clear and descriptive method names. This interface provides a common contract for different types of arguments, allowing for flexibility in implementing various argument types. This design choice aligns well with the goal of supporting both build arguments and potentially other types of arguments in the future.
25-39: LGTM: CustomArg struct and methods are well-implemented.The
CustomArgstruct and its methods correctly implement theArgInterface. The use of a type assertionvar _ ArgInterface = &CustomArg{}is a good practice to ensure the interface is properly implemented at compile-time.Consider adding a brief comment explaining the purpose of the type assertion:
// Ensure CustomArg implements ArgInterface var _ ArgInterface = &CustomArg{}Could you please clarify the intended use cases for
CustomArg? While it provides flexibility, it's not immediately clear from the PR objectives how it will be utilized. This information would be helpful for understanding the full scope of the implementation.pkg/builder/builder.go (1)
17-17: Approved change, but additional context needed.The change from
[]stringto[]ArgInterfacefor theArgsfield inBuilderOptionsaligns with the PR objective of adding support for build arguments. This structured approach should provide more flexibility in handling different types of arguments.However, could you please provide more information on the following:
- Where is
ArgInterfacedefined?- What methods does
ArgInterfacedeclare?- Are there any existing implementations of
ArgInterface?This information will help in understanding the full scope of this change and its impact on the rest of the codebase.
Let's verify the definition and usage of
ArgInterface:e2e/system/build_image_test.go (2)
5-5: LGTM: Import statement addition is appropriate.The addition of the "fmt" package import is necessary for the new functionality in the
TestBuildWithBuildArgsfunction.
Line range hint
1-134: Overall implementation aligns well with PR objectives.The changes in this file effectively implement and test the new build argument functionality for Docker image builds using Kaniko. The new
TestBuildWithBuildArgsfunction provides good coverage for the feature, and the modifications to the existing tests maintain consistency in the testing approach.The implementation aligns well with the PR objectives of adding support for build arguments, as outlined in issue #567. It allows for overriding specific variables during the build process, similar to Docker's
--build-argfeature.pkg/container/docker.go (3)
24-24: LGTM: Addition ofargsfield toBuilderFactoryThe addition of the
args []builder.ArgInterfacefield to theBuilderFactorystruct is a good approach to support build arguments. This change aligns well with the PR objective of enabling the passing of build-time variables.
28-28: LGTM: UpdatedNewBuilderFactoryto support build argumentsThe changes to the
NewBuilderFactoryfunction correctly implement the support for build arguments:
- The function signature now includes
args []builder.ArgInterface.- The
argsfield is properly initialized in the returnedBuilderFactory.These modifications align well with the PR objectives and enable the passing of build-time variables.
Also applies to: 38-38
Line range hint
1-189: Overall implementation looks good, but some clarification neededThe changes successfully implement support for build arguments in the
BuilderFactory,PushBuilderImage, andBuildImageFromGitRepomethods. This aligns well with the PR objective of adding support for build arguments when building Docker images using Kaniko.However, to fully meet the PR objectives, please clarify or address the following:
How are specific global variables like
MAX_SQUARE_SIZEandUPGRADE_HEIGHT_DELAYhandled or overridden using this implementation?Is there a need to add any validation or parsing of the build arguments to ensure they match the expected format for overriding these global variables?
Consider adding documentation or comments explaining how users can utilize this new functionality to override specific global variables, similar to the example provided in the linked issue:
docker build --build-arg MAX_SQUARE_SIZE=64 --build-arg UPGRADE_HEIGHT_DELAY=1000 -t celestia-app:latest .Addressing these points will ensure that the implementation fully meets the PR objectives and provides clear guidance for users.
pkg/builder/kaniko/kaniko.go (4)
146-149: LGTM: Improved code structure and argument handling.The changes to the variable declarations and the introduction of the
prepareArgsfunction improve code readability and maintainability. This refactoring aligns well with the PR objectives by providing a centralized mechanism for handling build arguments.Also applies to: 163-163
Line range hint
1-283: Overall feedback: Good implementation with some areas for improvementThe changes in this file successfully implement support for build arguments when building Docker images using Kaniko, aligning well with the PR objectives. The introduction of the
prepareArgsfunction improves code structure and maintainability.However, there are a few areas that need attention:
- Clarify how
MAX_SQUARE_SIZEandUPGRADE_HEIGHT_DELAYarguments are being handled.- Address or provide context for the TODO comments in the
prepareArgsfunction.- Consider adding more documentation to explain the purpose of different argument groups.
Once these points are addressed, the implementation will be more robust and easier to maintain in the future.
255-283: LGTM with suggestions: NewprepareArgsfunction.The new
prepareArgsfunction effectively centralizes the argument preparation logic and implements the handling of cache options and build arguments as per the PR objectives. However, there are a few points to consider:
The function doesn't explicitly handle the
MAX_SQUARE_SIZEandUPGRADE_HEIGHT_DELAYarguments mentioned in the PR objectives. Can you clarify how these specific arguments are being handled?There are several TODO comments in the function. Could you provide more context on these items and when they will be addressed?
Consider adding comments to explain the purpose of each argument group (context, destination, cache, etc.) for better readability.
To ensure that the
MAX_SQUARE_SIZEandUPGRADE_HEIGHT_DELAYarguments are being handled correctly, please run the following script:#!/bin/bash # Description: Verify that MAX_SQUARE_SIZE and UPGRADE_HEIGHT_DELAY are being handled in the codebase. # Test: Search for MAX_SQUARE_SIZE and UPGRADE_HEIGHT_DELAY in the codebase echo "Searching for MAX_SQUARE_SIZE:" rg --type go "MAX_SQUARE_SIZE" echo "Searching for UPGRADE_HEIGHT_DELAY:" rg --type go "UPGRADE_HEIGHT_DELAY" # Test: Check if these arguments are being passed to the kaniko builder echo "Checking if arguments are passed to kaniko builder:" rg --type go -A 10 "kaniko.*Build.*Options"
258-259: Address TODO comments and create follow-up tasks.There are several TODO comments in the
prepareArgsfunction that need attention:
- Git options (lines 258-259)
- Authentication token for the registry (lines 261-262)
- Authentication token for the cache repo (line 266)
These comments indicate important features that are yet to be implemented. To ensure these don't get overlooked:
- Could you provide more context on the requirements for these features?
- Consider creating separate GitHub issues for each of these items to track their implementation.
- If these features are not required for the current PR, please remove the TODO comments and address them in future PRs.
To get a better understanding of the current state of these features, please run the following script:
Also applies to: 261-262, 266-266
pkg/instance/build.go (4)
48-48: Addition of build arguments to 'SetImage' methodThe method
SetImagenow includes variadic argumentsargs ...builder.ArgInterface, enabling the passing of build arguments. This change aligns with the PR objectives to support build arguments when building Docker images using Kaniko.
57-57: Passing build arguments to 'NewBuilderFactory' in 'SetImage'The
argsare correctly passed tocontainer.NewBuilderFactory, allowing the builder to utilize the specified build arguments during the image build process.
69-69: Addition of build arguments to 'SetGitRepo' methodThe method
SetGitReponow includes variadic argumentsargs ...builder.ArgInterface, allowing build arguments to be passed when building images from a Git repository. This enhancement supports the PR's goal of adding build argument functionality.
83-83: Passing build arguments to 'NewBuilderFactory' in 'SetGitRepo'The
argsare appropriately passed tocontainer.NewBuilderFactory, ensuring that build arguments are utilized when building the image from the Git repository.
mojtaba-esk
deleted the
mojtaba/567-feat-support-for-build-arguments-in-kaniko
branch
Closes #567
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor