SKIP-1159 - Add recommended labels. #527
Conversation
… some parsing and "prettyfication" of version-string
WalkthroughThe changes involve enhancements to the Changes
Sequence Diagram(s)sequenceDiagram
participant Application
participant Routing
participant SKIPJob
participant SKIPNamespace
participant VersionLabel
Application->>Application: GetDefaultLabels()
Application->>VersionLabel: getVersionLabel(a.Spec.Image)
Application-->>Application: Return labels with name and version
Routing->>Routing: GetDefaultLabels()
Routing-->>Routing: Return labels with name
SKIPJob->>SKIPJob: GetDefaultLabels()
SKIPJob->>VersionLabel: getVersionLabel(skipJob.Spec.Container.Image)
SKIPJob-->>SKIPJob: Return labels with name and version
SKIPNamespace->>SKIPNamespace: GetDefaultLabels()
SKIPNamespace-->>SKIPNamespace: Return labels with name
Possibly related PRs
Suggested reviewers
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review due to trivial changes (1)
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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- api/v1alpha1/application_types.go (1 hunks)
- api/v1alpha1/routing_types.go (1 hunks)
- api/v1alpha1/skipjob_types.go (1 hunks)
- api/v1alpha1/skipns_types.go (1 hunks)
- api/v1alpha1/skipobj_interfaces.go (2 hunks)
Additional comments not posted (4)
api/v1alpha1/skipns_types.go (1)
21-21: Approved addition of new Kubernetes label.The addition of
"app.kubernetes.io/name": n.Nameto the labels map is consistent with the PR's objectives to enhance labeling for Kubernetes resources. Ensure that theNameproperty is correctly accessible from thecorev1.NamespacewhichSKIPNamespaceembeds.api/v1alpha1/routing_types.go (1)
111-111: Approved addition of new Kubernetes label.The addition of
"app.kubernetes.io/name": in.Nameto the labels map is consistent with the PR's objectives to enhance labeling for Kubernetes resources. Ensure that theNameproperty is correctly accessible from themetav1.ObjectMetawhichRoutingembeds.api/v1alpha1/skipjob_types.go (1)
271-272: Enhance Kubernetes metadata with new labels.The addition of
"app.kubernetes.io/name"and"app.kubernetes.io/version"labels in theGetDefaultLabelsmethod enhances Kubernetes metadata, aligning with best practices for application identification and versioning. Ensure that thegetVersionLabelfunction is implemented efficiently and correctly handles edge cases in version formatting.The changes are approved, but verify the implementation of
getVersionLabel.Verification successful
The
getVersionLabelfunction is implemented correctly.The
getVersionLabelfunction efficiently handles version strings by splitting the image string and defaulting to "latest" if no version is specified. This implementation aligns with typical Docker image tag conventions and handles edge cases appropriately. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `getVersionLabel` function. # Test: Search for the function implementation. Expect: Efficient and correct handling of version strings. rg --type python -A 5 $'getVersionLabel'Length of output: 70
Script:
#!/bin/bash # Description: Verify the implementation of `getVersionLabel` function in Go files. # Test: Search for the function implementation. Expect: Efficient and correct handling of version strings. rg --type go -A 5 'getVersionLabel'Length of output: 1596
api/v1alpha1/application_types.go (1)
419-420: Standardize Kubernetes labels across applications.The updated
GetDefaultLabelsmethod now includes standardized labels such as"app.kubernetes.io/name"and"app.kubernetes.io/version", which improve the clarity and manageability of Kubernetes resources. Consistency in label usage across different types (SKIPJob and Application) enhances the overall architecture. Again, ensure that thegetVersionLabelfunction is correctly implemented.The changes are approved, but verify the implementation of
getVersionLabel.
api/v1alpha1/skipobj_interfaces.go
Outdated
| func getVersionLabel(imageVersionString string) string { | ||
| parts := strings.Split(imageVersionString, ":") | ||
|
|
||
| // Implicitly assume version "latest" if no version is specified | ||
| if len(parts) < 2 { | ||
| return "latest" | ||
| } | ||
|
|
||
| versionPart := parts[1] | ||
|
|
||
| // Remove "@sha256" from version text if version includes a hash | ||
| if strings.Contains(versionPart, "@") { | ||
| versionPart = strings.Split(versionPart, "@")[0] | ||
| } | ||
|
|
||
| // Add build number to version if it is specified | ||
| if len(parts) > 2 { | ||
| return versionPart + "+" + parts[2] | ||
| } | ||
| return versionPart | ||
| } |
There was a problem hiding this comment.
Approved new function for version label processing.
The getVersionLabel function is a good addition for handling image version strings. It correctly handles cases where the version might be missing or includes a hash, and it appends a build number if specified. Consider adding unit tests to cover various scenarios and edge cases to ensure robustness.
Would you like me to help generate unit tests for this function?
…ts and integration-tests for both Application and SKIPJob
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
pkg/resourcegenerator/resourceutils/helpers.go (1)
21-41: Add error handling and validation to improve the function.The function can be further improved by adding the following:
- Error handling for invalid input, such as an empty string or a string with an invalid format.
- Validation of the version string format to ensure it follows the expected pattern.
Here's an example of how you can add error handling and validation:
func GetImageVersion(imageVersionString string) (string, error) { if imageVersionString == "" { return "", fmt.Errorf("empty image version string") } parts := strings.Split(imageVersionString, ":") if len(parts) < 2 { return "latest", nil } versionPart := parts[1] if strings.Contains(versionPart, "@") { versionParts := strings.Split(versionPart, "@") if len(versionParts) != 2 { return "", fmt.Errorf("invalid version format: %s", versionPart) } versionPart = versionParts[0] } if len(parts) > 2 { buildNumber := parts[2] if !isValidBuildNumber(buildNumber) { return "", fmt.Errorf("invalid build number: %s", buildNumber) } return versionPart + "+" + buildNumber, nil } return versionPart, nil } func isValidBuildNumber(buildNumber string) bool { // Add your validation logic here // For example, check if the build number consists of only digits _, err := strconv.Atoi(buildNumber) return err == nil }In this example:
- The function returns an error if the input string is empty.
- The function validates the version format when splitting by "@" and returns an error if it's invalid.
- The function validates the build number using a separate
isValidBuildNumberfunction and returns an error if it's invalid.You can customize the validation logic based on your specific requirements.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- api/v1alpha1/application_types.go (1 hunks)
- api/v1alpha1/skipjob_types.go (1 hunks)
- pkg/resourcegenerator/deployment/deployment.go (1 hunks)
- pkg/resourcegenerator/job/job.go (4 hunks)
- pkg/resourcegenerator/resourceutils/helpers.go (2 hunks)
- pkg/resourcegenerator/resourceutils/helpers_test.go (1 hunks)
- pkg/resourcegenerator/resourceutils/metadata_test.go (1 hunks)
- pkg/resourcegenerator/service/service.go (2 hunks)
- tests/application/labels-imageversion/application-assert.yaml (1 hunks)
- tests/application/labels-imageversion/application-patch.yaml (1 hunks)
- tests/application/labels-imageversion/application.yaml (1 hunks)
- tests/application/labels-imageversion/chainsaw-test.yaml (1 hunks)
- tests/application/minimal/application-assert.yaml (5 hunks)
- tests/skipjob/minimal-job/skipjob-assert.yaml (2 hunks)
Files skipped from review due to trivial changes (3)
- tests/application/labels-imageversion/application-assert.yaml
- tests/application/minimal/application-assert.yaml
- tests/skipjob/minimal-job/skipjob-assert.yaml
Additional context used
yamllint
tests/application/labels-imageversion/application-patch.yaml
[error] 7-7: no new line character at the end of file
(new-line-at-end-of-file)
Additional comments not posted (17)
tests/application/labels-imageversion/application.yaml (1)
1-7: LGTM!The file defines a valid Kubernetes Application resource for testing purposes. The resource does not specify any labels, which is likely the expected behavior for this test case.
tests/application/labels-imageversion/application-patch.yaml (1)
1-7: LGTM! The YAML manifest follows the expected structure.The manifest defines a Kubernetes Application resource with the necessary fields:
apiVersionandkindare correctly set.metadata.nameis provided.spec.imageis using a SHA256 digest, which is a good practice for ensuring immutability and reproducibility of deployments.spec.portis set to a commonly used port 8080.Tools
yamllint
[error] 7-7: no new line character at the end of file
(new-line-at-end-of-file)
tests/application/labels-imageversion/chainsaw-test.yaml (1)
1-16: LGTM!The test definition looks good. The file is correctly defining the Chainsaw test resource with the necessary steps.
pkg/resourcegenerator/resourceutils/metadata_test.go (1)
33-44: LGTM!The code changes are approved. The new
expectedLabelsmap and the modified assertions improve the robustness of the test by ensuring that all expected labels are present and correctly assigned.The changes align with the suggestion from the past review comment by martinhny:
It would create a better test if you checked against an
expectedLabelsmap withassertEqual(sa.GetLabels, expectedLabels)for exampleThe past review comment has been addressed by these changes.
pkg/resourcegenerator/resourceutils/helpers_test.go (4)
8-15: LGTM!The test function is correctly implemented and tests the expected behavior of the
GetImageVersionfunction when the image string does not have a tag.
17-24: LGTM!The test function is correctly implemented and tests the expected behavior of the
GetImageVersionfunction when the image string has thelatesttag.
26-38: LGTM!The test function is correctly implemented and tests the expected behavior of the
GetImageVersionfunction when the image string has a version tag or a dev version tag.
40-47: LGTM!The test function is correctly implemented and tests the expected behavior of the
GetImageVersionfunction when the image string has a SHA tag.pkg/resourcegenerator/service/service.go (2)
8-8: LGTM!The code changes are approved.
40-40: LGTM!The code changes are approved.
pkg/resourcegenerator/job/job.go (4)
9-9: LGTM!The new import statement is necessary to use the
GetImageVersionfunction.
31-34: LGTM!The changes enhance the metadata associated with the job by adding a version label using the
GetImageVersionfunction. The changes look good.
79-79: LGTM!The changes enhance the metadata associated with the cron job by adding a version label using the
GetImageVersionfunction. The changes look good.
135-135: LGTM!The changes enhance the metadata associated with the job spec by adding a version label using the
GetImageVersionfunction. The changes look good.pkg/resourcegenerator/deployment/deployment.go (1)
111-111: LGTM!Adding the
app.kubernetes.io/versionlabel to the pod template is a good practice for tracking and managing application versions in Kubernetes deployments. The chosen label key follows the recommended Kubernetes label convention, and using theGetImageVersionfunction to extract the version from the image specification is a sensible approach.api/v1alpha1/skipjob_types.go (1)
271-271: LGTM!The new label
"app.kubernetes.io/name"is correctly set to theNameproperty of theskipJobinstance. This change aligns with the PR objective of adding recommended labels for the Kubernetes application.api/v1alpha1/application_types.go (1)
420-420: LGTM!The addition of the
app.kubernetes.io/namelabel enhances the metadata associated with the application and aligns with Kubernetes' labeling conventions. This change improves the application's integration with Kubernetes by providing clearer identification.
Added recommended labels for app.kubernetes.io/name and version, plus some parsing and "prettyfication" of version-string.
Summary by CodeRabbit
New Features
Bug Fixes