feat: add kubebuilders validations for name length to be <= 63 #218

jamyct · 2024-11-15T12:32:05Z

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Requirements:

run make reviewable for basic local test
Read and followed fleet-networking's Code of conduct.
uses conventional commit messages
includes documentation
adds unit tests

How has this code been tested

Special notes for your reviewer

codecov · 2024-11-15T12:42:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.26%. Comparing base (7a16d0c) to head (c0acd83).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #218      +/-   ##
==========================================
+ Coverage   80.10%   80.26%   +0.15%     
==========================================
  Files          27       27              
  Lines        3840     3840              
==========================================
+ Hits         3076     3082       +6     
+ Misses        610      603       -7     
- Partials      154      155       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

config/rbac/role.yaml

test/apis/v1alpha1/api_validation_integration_test.go

doc

fix: requeue the request when the dns is not assigned

Bumps [thehanimo/pr-title-checker](https://github.com/thehanimo/pr-title-checker) from 1.4.2 to 1.4.3. - [Release notes](https://github.com/thehanimo/pr-title-checker/releases) - [Commits](thehanimo/pr-title-checker@v1.4.2...v1.4.3) --- updated-dependencies: - dependency-name: thehanimo/pr-title-checker dependency-type: direct:production update-type: version-update:semver-patch ...

test/apis/v1alpha1/api_validation_integration_test.go

Makefile

merge with latest

Makefile

test/apis/v1alpha1/suite_test.go

test/apis/v1alpha1/api_validation_integration_test.go

zhiying-lin

Please apply the comments to all the APIs tests.

zhiying-lin · 2024-12-12T11:39:33Z

Makefile

@@ -110,8 +110,9 @@ local-unit-test: $(ENVTEST) ## Run tests.

 .PHONY: integration-test
 integration-test: $(ENVTEST) ## Run integration tests.
+	go install github.com/onsi/ginkgo/v2/[email protected]


please install the ginko in the .github/workflows/unit-integration-tests.yml similar to what fleet repo does.
https://github.com/Azure/fleet/blob/main/.github/workflows/ci.yml#L47

zhiying-lin · 2024-12-12T11:40:44Z

test/apis/v1alpha1/api_validation_integration_test.go

 			Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create API call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8serrors.StatusError{})))
 			Expect(statusErr.Status().Message).Should(ContainSubstring("metadata.name max length is 63"))
 		})

 		It("should deny creating API with invalid name starting with non-alphanumeric character", func() {
-			var name = "-abcdef-123456789-123456789-123456789-123456789-123456789"
+			var nameStartingWithNonAlphanum = "-abcdef-123456789-123456789-123456789-123456789-123456789"


please make it as a global variable, so that it can be shared between all the resources.

zhiying-lin · 2024-12-12T11:41:15Z

test/apis/v1alpha1/suite_test.go

+	ctx                 context.Context
+	cancel              context.CancelFunc
+	testNamespace       = "testnamespace"
+	nameWithInvalidSize = "abcdef-123456789-123456789-123456789-123456789-123456789-123456789-123456789"


please declare this varible in api_validation_integration_test.go file where you used it.

zhiying-lin · 2024-12-12T11:41:35Z

test/apis/v1alpha1/suite_test.go

+			Name: testNamespace,
+		},
+	}
+	Expect(hubClient.Delete(ctx, &ns)).Should(Succeed())


Suggested change

Expect(hubClient.Delete(ctx, &ns)).Should(Succeed())

Expect(hubClient.Delete(ctx, &ns)).Should(Succeed(), failed to delete namespace)

zhiying-lin · 2024-12-12T11:41:43Z

test/apis/v1alpha1/suite_test.go

 		},
 	}
-	Expect(hubClient.Create(ctx, &ns)).Should(Succeed())
+	Expect(hubClient.Create(ctx, &ns)).Should(Succeed(), "failed to delete namespace")


Suggested change

Expect(hubClient.Create(ctx, &ns)).Should(Succeed(), "failed to delete namespace")

Expect(hubClient.Create(ctx, &ns)).Should(Succeed(), "failed to create namespace")

zhiying-lin · 2024-12-12T11:43:21Z

test/apis/v1alpha1/api_validation_integration_test.go

@@ -133,13 +131,13 @@ var _ = Describe("Test networking v1alpha1 API validation", func() {
 		})

 		It("should allow creating API with valid name starting with alphabet character", func() {
-			var name = "abc-123456789"
+			var nameValid = "abc-123456789"


Please create the global variable nameValidStartingWithAphabet = "abc-123456789", so that we can distinguish them and reuse for all the resources.

zhiying-lin · 2024-12-12T11:47:58Z

test/apis/v1alpha1/api_validation_integration_test.go

+					},
+				},
+			}
+			Expect(hubClient.Create(ctx, multiClusterServiceName)).Should(Succeed())


Suggested change

Expect(hubClient.Create(ctx, multiClusterServiceName)).Should(Succeed())

Expect(hubClient.Create(ctx, multiClusterServiceName)).Should(Succeed(), "failed to create multiClusterService")

missed this before, feel better to add the debug information here

zhiying-lin · 2024-12-12T11:48:18Z

test/apis/v1alpha1/api_validation_integration_test.go

+				},
+			}
+			Expect(hubClient.Create(ctx, multiClusterServiceName)).Should(Succeed())
+			Expect(hubClient.Delete(ctx, multiClusterServiceName)).Should(Succeed())


Suggested change

Expect(hubClient.Delete(ctx, multiClusterServiceName)).Should(Succeed())

Expect(hubClient.Delete(ctx, multiClusterServiceName)).Should(Succeed(), "failed to delete multiClusterService")

jamyct changed the title ~~add kubebuilders validations for name length to be <= 63~~ feat: add kubebuilders validations for name length to be <= 63 Nov 15, 2024

jamyct added the enhancement New feature or request label Nov 20, 2024

jamyct added 6 commits November 20, 2024 13:05

add kubebuilders validations for name length to be <= 63

cabe958

add validation tests for member cluster service

d7110d4

delete e2e test file

72bcdd7

add tests for service export

cd5ab5f

add service import tests

8907ad1

add tests for service import and traffic manager profile/backend

c730655

jamyct force-pushed the add-resource-validation branch from a87267d to c730655 Compare November 20, 2024 02:13

jamyct added 13 commits November 20, 2024 17:09

add integration test to makefile

f5faf85

set TimeoutInSeconds default to 9

e6ac7e8

add conditions for MonitorConfig

5673aef

add message to rule

4f3a8df

add second values to monitorconfig

e2faf02

run goimports

f6c87ba

resolve type comparison issue in CEL

0630e47

add case fo when intervalInSeconds is not 30 or 10

a5d9fac

when IntervalInSeconds is 10, TimeoutInSeconds should be between 5 & 9

9023bdf

add integration-test to test

40ea19a

test all integration potential files

dccfcae

remove additional conditions

08f1c64

remove role.yaml changes

41ac502

zhiying-lin reviewed Dec 3, 2024

View reviewed changes

jamyct and others added 6 commits December 3, 2024 13:37

fix filepath in hubtestenv

50f4a8f

add copyright banner, fix format and change wording

679cdea

fix: fix the atm feature related bugs (Azure#220)

0217227

docs: add application migration tutorials (Azure#222)

e73a06d

doc

fix: requeue the request when the dns is not assigned (Azure#225)

b6652ae

fix: requeue the request when the dns is not assigned

delete e2e resource validation test

5d27f30

zhiying-lin reviewed Dec 3, 2024

View reviewed changes

test/apis/v1alpha1/api_validation_integration_test.go Outdated Show resolved Hide resolved

zhiying-lin marked this pull request as draft December 6, 2024 06:10

jamyct added 2 commits December 11, 2024 12:39

add namespace in suit_test.go

cd95d76

add namespace in suit_test.go

d08d860

zhiying-lin reviewed Dec 11, 2024

View reviewed changes

Makefile Show resolved Hide resolved

jamyct added 12 commits December 11, 2024 17:49

fix directory for integration test

391e833

update integration tests to use names longer than 63

5a05046

make names valid

638c5c6

fix error message substring check for invalid RFC 1123 names

9a0c3a9

fix naming to multiClusterServiceName

96d7da9

remove example test files

cf66ebe

copyright banner

46fc413

remove integration coverage xml file

1f27453

fix import format in suite_test.go

577a447

edit format

c008933

Merge remote-tracking branch 'origin/main' into add-resource-validation

ed2feec

merge with latest

undo edit

c384093

jamyct marked this pull request as ready for review December 12, 2024 02:55

zhiying-lin reviewed Dec 12, 2024

View reviewed changes

jamyct added 9 commits December 12, 2024 17:27

delete print error and add error message if namespace not created

4ee9956

rename fleetnetv1alpha1 for better readability

bfd40ec

use global variable for namespace string

2f7e32a

delete namespace in aftersuite

4d29d65

use ginkgo to run integration test

1308cd3

fix context

e0211da

fix names of strings to test

bc21a2f

create global variable for nameWithInvalidSize

2c83da8

install ginkgo

c0acd83

zhiying-lin reviewed Dec 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add kubebuilders validations for name length to be <= 63 #218

feat: add kubebuilders validations for name length to be <= 63 #218

jamyct commented Nov 15, 2024 •

edited

Loading

codecov bot commented Nov 15, 2024 •

edited

Loading

zhiying-lin left a comment

zhiying-lin Dec 12, 2024

zhiying-lin Dec 12, 2024

zhiying-lin Dec 12, 2024

zhiying-lin Dec 12, 2024

zhiying-lin Dec 12, 2024

zhiying-lin Dec 12, 2024

zhiying-lin Dec 12, 2024

zhiying-lin Dec 12, 2024

	Expect(hubClient.Delete(ctx, &ns)).Should(Succeed())
	Expect(hubClient.Delete(ctx, &ns)).Should(Succeed(), failed to delete namespace)

	Expect(hubClient.Create(ctx, &ns)).Should(Succeed(), "failed to delete namespace")
	Expect(hubClient.Create(ctx, &ns)).Should(Succeed(), "failed to create namespace")

	Expect(hubClient.Create(ctx, multiClusterServiceName)).Should(Succeed())
	Expect(hubClient.Create(ctx, multiClusterServiceName)).Should(Succeed(), "failed to create multiClusterService")

	Expect(hubClient.Delete(ctx, multiClusterServiceName)).Should(Succeed())
	Expect(hubClient.Delete(ctx, multiClusterServiceName)).Should(Succeed(), "failed to delete multiClusterService")

feat: add kubebuilders validations for name length to be <= 63 #218

Are you sure you want to change the base?

feat: add kubebuilders validations for name length to be <= 63 #218

Conversation

jamyct commented Nov 15, 2024 • edited Loading

How has this code been tested

Special notes for your reviewer

codecov bot commented Nov 15, 2024 • edited Loading

Codecov Report

zhiying-lin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamyct commented Nov 15, 2024 •

edited

Loading

codecov bot commented Nov 15, 2024 •

edited

Loading