Followup ssatag #122
Followup ssatag #122
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
|
/assign @JoelSpeed @sbueringer |
| ByteArrayAliased ByteArrayAlias `json:"byteArrayAliased,omitempty"` | ||
|
|
||
| // Byte array with markers - should be ignored even if markers are present | ||
| // +listType=atomic |
There was a problem hiding this comment.
But it's better than the current behavior 😀
There was a problem hiding this comment.
It might be good if the field type is byte array, then it doesn't have listType marker, it's ok to ignore. On the other hand, if the field is byte array, but it has listType marker, it might have to report it.
I want to hear @JoelSpeed 's opinion.
There was a problem hiding this comment.
How is a byte array represented in the CRD yaml? As string?
There was a problem hiding this comment.
byte array represents as base64 encoded string. So if it has listType marker, it doesn't make sense, do it ?
There was a problem hiding this comment.
It doesn't make sense to have a listType on a byte array since the generated schema is type: string format: byte. If we see a []byte with a listType we should suggest it to be removed IMO
There was a problem hiding this comment.
Probably fine to address in a follow-up though? (to get the SSA linter working at least with this PR :))
There was a problem hiding this comment.
Yeah I'm happy to have it followed up on if that's easier
There was a problem hiding this comment.
I can follow up it in this PR. Let me add the feature.
pkg/analysis/ssatags/analyzer.go
Outdated
| return &analysis.Analyzer{ | ||
| Name: name, | ||
| Name: name, | ||
|
|
k8s-ci-robot
added
size/XL
sivchari
force-pushed
the
followup-ssatag
branch
from
c03cf35 to
70ecc21
Compare
pkg/analysis/ssatags/analyzer.go
Outdated
| for _, marker := range listTypeMarkers { | ||
| pass.Report(analysis.Diagnostic{ | ||
| Pos: field.Pos(), | ||
| Message: fmt.Sprintf("%s is a byte array, which is not supported by Server-Side Apply. Remove the listType marker", utils.FieldName(field)), |
There was a problem hiding this comment.
You can apply a byte array with server side apply, it just has a different format to other arrays. This would probably make more sense
| Message: fmt.Sprintf("%s is a byte array, which is not supported by Server-Side Apply. Remove the listType marker", utils.FieldName(field)), | |
| Message: fmt.Sprintf("%s is a byte array, which is does not support the listType marker. Remove the listType marker", utils.FieldName(field)), |
Signed-off-by: sivchari <[email protected]> fix marker Signed-off-by: sivchari <[email protected]> add IsByteArray func Signed-off-by: sivchari <[email protected]> fix analyzer and test Signed-off-by: sivchari <[email protected]> fix lint Signed-off-by: sivchari <[email protected]> remove typeList marker if the byte array has it Signed-off-by: sivchari <[email protected]> fix report message Signed-off-by: sivchari <[email protected]>
sivchari
force-pushed
the
followup-ssatag
branch
from
79868df to
9b814cb
Compare
|
/approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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
4 participants
close #117
close #118
close #119
@JoelSpeed @sbueringer
I fix the marker, then check whether the element of array is byte or not.