unify marker definitions to markers package #74
Conversation
k8s-ci-robot
added
the
kind/cleanup
k8s-ci-robot
added
cncf-cla: yes
There was a problem hiding this comment.
I wonder if this really belongs in this package? The package itself is about extracting markers rather than containing the constants of markers used in the project.
I agree with doing this (was actually thinking about it this morning), but perhaps we want to have a sepate consts style package for containing these marker constants?
There was a problem hiding this comment.
I've been just thinking where the location is better than under helpers. And I decided to make the markers package under pkg directory, then reflect it in the latest commit.
| @@ -158,3 +152,7 @@ func hasStatusField(sTyp *ast.StructType, jsonTags extractjsontags.StructFieldTa | |||
|
|
|||
| return false | |||
| } | |||
|
|
|||
| func kubebuilderFormatWithValue(value string) string { | |||
There was a problem hiding this comment.
This should probably mention that it's the root marker, or have an option to pass in a marker?
There was a problem hiding this comment.
These formatted functions are also unified as Format.
sivchari
force-pushed
the
unify-marker-definition
branch
from
b6233b0 to
ecb63f5
Compare
pkg/markers/markers.go
Outdated
| KubebuilderItemsFormatMarker = "kubebuilder:validation:items:Format" | ||
| ) | ||
|
|
||
| func Format(marker, value string) string { |
There was a problem hiding this comment.
Maybe FormatMarkerWithValue?
pkg/markers/markers.go
Outdated
| ) | ||
|
|
||
| func Format(marker, value string) string { | ||
| return fmt.Sprintf("%s:=%s", marker, value) |
There was a problem hiding this comment.
Is this format always correct? What about markers that have multiple parameters like kubebuilder:validation:XValidation?
There was a problem hiding this comment.
In addition, kubebuilder supports := and =, then the enum uses ; to concat value, but multiuple parameter uses , to do. So it's not time to determine this API. Reverted it once.
k8s-ci-robot
added
the
needs-rebase
sivchari
force-pushed
the
unify-marker-definition
branch
2 times, most recently
from
f3a0ba8 to
4a8844a
Compare
k8s-ci-robot
removed
the
needs-rebase
| func kubebuilderFormatWithValue(format string) string { | ||
| return fmt.Sprintf("%s:=%s", markers.KubebuilderRootMarker, format) | ||
| } |
There was a problem hiding this comment.
I think this would be better. Note though, that not all markers support the := syntax, so this isn't universal. Anything with Kubebuilder in the name does support := though
| func kubebuilderFormatWithValue(format string) string { | |
| return fmt.Sprintf("%s:=%s", markers.KubebuilderRootMarker, format) | |
| } | |
| func formatKubeBuilderMarkerWithValue(marker, value string) string { | |
| return fmt.Sprintf("%s:=%s", marker, format) | |
| } |
|
If we can apply my suggestion then I'm good to get this merged, thanks for working on this one! |
Signed-off-by: sivchari <shibuuuu5@gmail.com>
sivchari
force-pushed
the
unify-marker-definition
branch
from
4a8844a to
4de5446
Compare
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven, JoelSpeed, sivchari The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Each linter definitions marker constant in themself, then there are some duplicated markers. In the future, we should add other marker definition to diagnose markers. So it's good to determine the location to define marker right now.
/kind cleanup