Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SKIP-1159 - A better labeler #557

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+63 −49
Open

SKIP-1159 - A better labeler #557

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cb16c11
Switch to non-illegal character in label-value
SnorreSelmer Nov 5, 2024
85fe06f
Added more test-cases
SnorreSelmer Nov 5, 2024
8e11abe
- Added more test-cases (thanks @evenh)
SnorreSelmer Nov 5, 2024
e382b06
- Rearranged some of the imageVersionString processing to make it mor…
SnorreSelmer Nov 5, 2024
eb0ab78
- First-character check now uses actual regex-matching
SnorreSelmer Nov 6, 2024
63f32df
Don't export matchesRegex function
SnorreSelmer Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 30 additions & 9 deletions pkg/resourcegenerator/resourceutils/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@ package resourceutils
import (
skiperatorv1alpha1 "github.com/kartverket/skiperator/api/v1alpha1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"regexp"
"strings"
)

const LabelValueMaxLength int = 63
SnorreSelmer marked this conversation as resolved.
Show resolved Hide resolved

func ShouldScaleToZero(jsonReplicas *apiextensionsv1.JSON) bool {
replicas, err := skiperatorv1alpha1.GetStaticReplicas(jsonReplicas)
if err == nil && replicas == 0 {
Expand All @@ -18,24 +21,42 @@ func ShouldScaleToZero(jsonReplicas *apiextensionsv1.JSON) bool {
return false
}

// MatchesRegex checks if a string matches a regexp
func MatchesRegex(s string, pattern string) bool {
SnorreSelmer marked this conversation as resolved.
Show resolved Hide resolved
obj, err := regexp.Match(pattern, []byte(s))
return obj && err == nil
}

// GetImageVersion returns the version part of an image string
func GetImageVersion(imageVersionString string) string {
SnorreSelmer marked this conversation as resolved.
Show resolved Hide resolved
parts := strings.Split(imageVersionString, ":")
// Find position of first "@", remove it and everything after it
if strings.Contains(imageVersionString, "@") {
imageVersionString = strings.Split(imageVersionString, "@")[0]
imageVersionString = imageVersionString + ":unknown"
SnorreSelmer marked this conversation as resolved.
Show resolved Hide resolved
}

// Implicitly assume version "latest" if no version is specified
if len(parts) < 2 {
// If no version is given, assume "latest"
if !strings.Contains(imageVersionString, ":") {
return "latest"
}
Comment on lines +39 to 41
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we return the image digest if no version is specified instead of latest?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't necessarily have the digest and for a human readable version I prefer latest.


// Split image string into parts
parts := strings.Split(imageVersionString, ":")

Comment on lines +44 to +45
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle image strings with multiple colons

Splitting imageVersionString by ":" and accessing parts[1] can lead to errors if the image string contains multiple colons, such as when specifying a registry with a port number (e.g., "registry:5000/repo/image:tag"). This could cause versionPart to be incorrect.

Apply this diff to correctly extract the version tag using the last occurrence of ":":

- parts := strings.Split(imageVersionString, ":")
- versionPart := parts[1]
+ lastColon := strings.LastIndex(imageVersionString, ":")
+ if lastColon == -1 || lastColon == len(imageVersionString)-1 {
+     versionPart := "latest"
+ } else {
+     versionPart := imageVersionString[lastColon+1:]
+     imageVersionString = imageVersionString[:lastColon]
+ }

This approach ensures that the version tag is accurately extracted, even when the image string includes a registry port or multiple colons.

Committable suggestion skipped: line range outside the PR's diff.

versionPart := parts[1]
SnorreSelmer marked this conversation as resolved.
Show resolved Hide resolved

// Remove "@sha256" from version text if version includes a hash
if strings.Contains(versionPart, "@") {
versionPart = strings.Split(versionPart, "@")[0]
// Replace "+" with "-" in version text if version includes one
versionPart = strings.ReplaceAll(versionPart, "+", "-")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should match any special character really 🤔


// Limit label-value to 63 characters
if len(versionPart) > LabelValueMaxLength {
versionPart = versionPart[:LabelValueMaxLength]
}
SnorreSelmer marked this conversation as resolved.
Show resolved Hide resolved

// Add build number to version if it is specified
if len(parts) > 2 {
return versionPart + "+" + parts[2]
// While first character is not part of regex [a-z0-9A-Z] then remove it
for len(versionPart) > 0 && !MatchesRegex(versionPart[:1], "[a-zA-Z0-9]") {
SnorreSelmer marked this conversation as resolved.
Show resolved Hide resolved
versionPart = versionPart[1:]
}
SnorreSelmer marked this conversation as resolved.
Show resolved Hide resolved

return versionPart
}
73 changes: 33 additions & 40 deletions pkg/resourcegenerator/resourceutils/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,47 +1,40 @@
package resourceutils

import (
"github.com/stretchr/testify/assert"
"testing"
)

func TestGetImageVersionNoTag(t *testing.T) {
imageString := "image"
expectedImageString := "latest"

actualImageString := GetImageVersion(imageString)

assert.Equal(t, expectedImageString, actualImageString)
}

func TestGetImageVersionLatestTag(t *testing.T) {
imageString := "image:latest"
expectedImageString := "latest"

actualImageString := GetImageVersion(imageString)

assert.Equal(t, expectedImageString, actualImageString)
}

func TestGetImageVersionVersionTag(t *testing.T) {
versionImageString := "image:1.2.3"
devImageString := "image:1.2.3-dev-123abc"
expectedVersionImageString := "1.2.3"
expectedDevImageString := "1.2.3-dev-123abc"

actualVersionImageString := GetImageVersion(versionImageString)
actualDevImageString := GetImageVersion(devImageString)

assert.Equal(t, expectedVersionImageString, actualVersionImageString)
assert.Equal(t, expectedDevImageString, actualDevImageString)

}

func TestGetImageVersionShaTag(t *testing.T) {
imageString := "ghcr.io/org/repo@sha256:54d7ea8b48d0e7569766e0e10b9e38da778a5f65d764168dd7db76a37d6b8"
expectedImageString := "54d7ea8b48d0e7569766e0e10b9e38da778a5f65d764168dd7db76a37d6b8"

actualImageString := GetImageVersion(imageString)
"github.com/stretchr/testify/assert"
)

assert.Equal(t, expectedImageString, actualImageString)
func TestVersions(t *testing.T) {
testCases := []struct {
imageString string
expectedValue string
}{
{"image", "latest"},
{"image:latest", "latest"},
{"image:1.2.3-dev-123abc", "1.2.3-dev-123abc"},
{"image:1.2.3", "1.2.3"},
{"ghcr.io/org/repo@sha256:54d7ea8b48d0e7569766e0e10b9e38da778a5f65d764168dd7db76a37d6b8", "unknown"},
{"ghcr.io/org/one-app:sha-b15dc91c27ad2387bea81294593d5ce5a686bcc4@sha256:3cda54f1d25458f25fdde0398130da57a4ebb4a4cd759bc49035b7ebf9d83619", "sha-b15dc91c27ad2387bea81294593d5ce5a686bcc4"},
{"ghcr.io/org/another-app:3fb7048", "3fb7048"},
{"ghcr.io/org/some-team/third-app:v1.2.54", "v1.2.54"},
{"ghcr.io/org/another-team/fourth-app:4.0.0.rc-36", "4.0.0.rc-36"},
{"ghcr.io/org/another-team/fifth-app:4.0.0.rc-36-master-latest", "4.0.0.rc-36-master-latest"},
{"ghcr.io/kartverket/vulnerability-disclosure-program@sha256:ab85022d117168585bdedc71cf9c67c3ca327533dc7cd2c5bcc42a83f308ea5d", "unknown"},
{"ghcr.io/kartverket/vulnerability-disclosure-program:4.0.1@sha256:ab85022d117168585bdedc71cf9c67c3ca327533dc7cd2c5bcc42a83f308ea5d", "4.0.1"},
{"nginxinc/nginx-unprivileged:1.20.0-alpine", "1.20.0-alpine"},
{"foo/bar:1.2.3+build.4", "1.2.3-build.4"},
{"foo/bar:1.2.3+somethingLongXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX", "1.2.3-somethingLongXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"},
{"foo/bar:-1.2.3", "1.2.3"},
{"foo/bar:__1.2.3", "1.2.3"},
{"foo/bar:.1.2.3", "1.2.3"},
}
Comment on lines +14 to +32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Documentation needs improvement for version string transformations

The implementation in GetImageVersion includes several important transformations that should be better documented in the function's header comment:

  • Handling of SHA digests (returns "unknown")
  • Replacing "+" with "-" in version strings
  • Truncating version strings longer than LabelValueMaxLength
  • Stripping non-alphanumeric leading characters
  • Default "latest" tag handling

The current comment // GetImageVersion returns the version part of an image string is too brief given the complexity of the transformations.

🔗 Analysis chain

Comprehensive test coverage with clear test cases!

The test cases cover a wide range of scenarios including:

  • Basic image tags
  • Semantic versions
  • SHA digests
  • Special character handling
  • Version string transformations

Let's verify if these transformations are documented in the implementation:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the version string transformation rules are documented
# Expected: Comments or documentation explaining the transformation rules

# Look for documentation about version transformations
rg -A 5 "GetImageVersion" pkg/resourcegenerator/resourceutils/helpers.go

Length of output: 429

Script:

#!/bin/bash
# Let's see the complete implementation of GetImageVersion to understand all transformations
rg -A 20 "GetImageVersion" pkg/resourcegenerator/resourceutils/helpers.go

# Also check for any related tests that might document the behavior
rg -A 10 "TestGetImageVersion" pkg/resourcegenerator/resourceutils/helpers_test.go

Length of output: 901

Script:

#!/bin/bash
# Let's try with ast-grep to get the complete function implementation
ast-grep --pattern 'func GetImageVersion($_) string {
  $$$
}'

# And get the test function as well
ast-grep --pattern 'func TestGetImageVersion($_) $_ {
  $$$
}'

Length of output: 2719


for _, tc := range testCases {
t.Run(tc.imageString, func(t *testing.T) {
actualValue := GetImageVersion(tc.imageString)
assert.Equal(t, tc.expectedValue, actualValue)
})
}
}
Loading