Unsupported keys per provider #324
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
#206 is blocked on this. |
|
Can we have unit tests for this, please ? |
|
@sebgoa for sure. It is not going to be merged without it. |
|
Changes Unknown when pulling fcb8352 on rtnpro:unsupported-keys-per-provider into ** on kubernetes-incubator:master**. |
|
|
||
| // KomposeObject holds the generic struct of Kompose transformation | ||
| type KomposeObject struct { | ||
| ServiceConfigs map[string]ServiceConfig | ||
| // name of the loader that was created KomposeObject | ||
| LoadedFrom string |
There was a problem hiding this comment.
Transformer need to know origin format in order to tell user what tag is not supported in origin format. They can have different names.
for example environment variables. are called environment in compose,Env in bundle.
There was a problem hiding this comment.
so this explanation in code comment can help!
| User string `compose:"user",bundle:"User"` | ||
| VolumesFrom []string `compose:"volumes_from",bundle:""` | ||
| ServiceType string `compose:"kompose.service.type",bundle:""` | ||
| Build string `compose:"build",bundle:""` |
There was a problem hiding this comment.
why are tags needed for ServiceConfig ? Is this for structs?
There was a problem hiding this comment.
So we know from what element those values loaded by loader. (more in my reply to LoadedFrom)
Plus in future we can use this to automatically generate docs explaining transformation.
| } | ||
| } | ||
|
|
||
| if counter, ok := unsupportedKey[f.Name()]; ok { |
There was a problem hiding this comment.
What if we can get this as the first check in this for loop for _, f := range s.Fields(), then it will save a lot of processing that happens before coming here? Similarly in other checkUnsupportedKey?
There was a problem hiding this comment.
Because this particular check decides if that particular key is supported or not?
There was a problem hiding this comment.
Technically It could be moved there, I kept it in the same order as it was before.
I wouldn't say a lot of processing as happening before. It is all just simple checks. Plus it is not like there will ever be hundreds of keys.
But I updated it anyway ;-) Plus I changed int counter in unsuportedKeys to bool.
Because all we need is to know if there already was that key or not ;-)
| // this check if field is Slice, and then it checks its size | ||
| if field := val.FieldByName(f.Name()); field.Kind() == reflect.Slice { | ||
| if field.Len() == 0 { | ||
| // array is empty it doesn't metter if it is in unsupportedKey or not |
| // this check if field is Slice, and then it checks its size | ||
| if field := val.FieldByName(f.Name()); field.Kind() == reflect.Slice { | ||
| if field.Len() == 0 { | ||
| // array is empty it doesn't metter if it is in unsupportedKey or not |
| // this check if field is Slice, and then it checks its size | ||
| if field := val.FieldByName(f.Name()); field.Kind() == reflect.Slice { | ||
| if field.Len() == 0 { | ||
| // array is empty it doesn't metter if it is in unsupportedKey or not |
|
Changes Unknown when pulling e1d88f7 on rtnpro:unsupported-keys-per-provider into ** on kubernetes-incubator:master**. |
kadel
force-pushed
the
unsupported-keys-per-provider
branch
2 times, most recently
from
ed74bd2 to
1e5d5e3
Compare
|
@ngtuna your say here will be appreciated! |
kadel
force-pushed
the
unsupported-keys-per-provider
branch
from
1e5d5e3 to
2e40f45
Compare
rtnpro
force-pushed
the
unsupported-keys-per-provider
branch
from
2e40f45 to
e3ef748
Compare
rtnpro
force-pushed
the
unsupported-keys-per-provider
branch
from
e3ef748 to
3419ae7
Compare
|
@surajssd rebased and ready for merging. |
Separate unsupported keys based on provider.