Add validation for ClusterRepo #306
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rohitsakala
force-pushed
the
release/v0.4
branch
from
f4d58c3
to
1b4c44f
Compare
pjbgf
reviewed
Oct 27, 2023
maxsokolovsky
requested changes
Oct 27, 2023
pkg/resources/catalog.cattle.io/v1/clusterrepo/validator_test.go
Outdated
Show resolved
Hide resolved
pkg/resources/catalog.cattle.io/v1/clusterrepo/validator_test.go
Outdated
Show resolved
Hide resolved
rohitsakala
force-pushed
the
release/v0.4
branch
3 times, most recently
from
41b859c
to
a07ab82
Compare
rohitsakala
force-pushed
the
release/v0.4
branch
from
a07ab82
to
f210e2a
Compare
maxsokolovsky
requested changes
Nov 1, 2023
rohitsakala
force-pushed
the
release/v0.4
branch
from
f210e2a
to
5232c8b
Compare
rohitsakala
force-pushed
the
release/v0.4
branch
from
5232c8b
to
3d67bf3
Compare
rohitsakala
force-pushed
the
release/v0.4
branch
from
3d67bf3
to
7eb0f4d
Compare
pjbgf
reviewed
Nov 2, 2023
rohitsakala
force-pushed
the
release/v0.4
branch
from
7eb0f4d
to
e37fd5d
Compare
rohitsakala
force-pushed
the
release/v0.4
branch
from
e37fd5d
to
e37659b
Compare
maxsokolovsky
requested changes
Nov 2, 2023
| return admission.ResponseAllowed(), nil | ||
| } | ||
|
|
||
| func (a *admitter) validateFields(newClusterrepo *catalogv1.ClusterRepo) error { |
There was a problem hiding this comment.
Nit
Suggested change
| func (a *admitter) validateFields(newClusterrepo *catalogv1.ClusterRepo) error { | |
| func (a *admitter) validateFields(repo *catalogv1.ClusterRepo) error { |
There was a problem hiding this comment.
why should I change it from newClusterRepo to repo? The resource name is ClusterRepo not just repo. Also,the new signifies it is the latest one.
There was a problem hiding this comment.
It's up to you, hence I marked it as a nit. At a minimum, you might want to make the casing look better, so newClusterRepo instead of newClusterrepo.
But you sort of repeat this composite name after the type name, which reads strangely. And it doesn't matter whether the repo is the latest one, you simply validate its fields. But it's all up to you.
rohitsakala
force-pushed
the
release/v0.4
branch
from
e37659b
to
6e552f4
Compare
maxsokolovsky
approved these changes
Nov 3, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Github Issue - https://github.com/rancherlabs/rancher-security/issues/1258
Add validation and mutation for ClusterRepo
New validations:
Both GitRepo and URl cannot exist simultaneously
Testing
Added unit tests