init: Add default config name #2668
Conversation
corneliusweig
requested review from
balopat,
briandealwis,
chanseokoh,
dgageot,
loosebazooka,
nkubala,
priyawadhwa,
TadCordle and
tejal29
as code owners
Codecov Report
|
|
@balopat That's a good point. At the moment, we don't have any restrictions for config names. But usual kubernetes resources have some constraints (from https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names):
Even though we don't validate that right now, I think it's a good idea to follow this pattern. What do you think about |
|
@corneliusweig seems reasonable to me, better safe than sorry. |
|
@corneliusweig bump 👍 |
pkg/skaffold/initializer/init.go
Outdated
| @@ -372,9 +374,15 @@ func generateSkaffoldConfig(k Initializer, buildConfigPairs []builderImagePair) | |||
| // if the user doesn't have any k8s yamls, generate one for each dockerfile | |||
| logrus.Info("generating skaffold config") | |||
|
|
|||
| name, err := suggestConfigName() | |||
| if err != nil { | |||
| return nil, errors.Wrap(err, "generating default config name") | |||
There was a problem hiding this comment.
should this be an error ? since its a optional field, we should log this as error and continue
There was a problem hiding this comment.
Yes, I agree.
|
@tejal29 I'm wondering if it even was a good idea to add the field in our examples. So far, they have been quite minimal and this breaks with that rationale. Please let me know if you have the same doubts. If you think this is a good idea, I'll create the separate PR, but I'll drop the commit from here. |
Defaults to the basename of the current workdir. Signed-off-by: Cornelius Weig <[email protected]>
See https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names for details. Signed-off-by: Cornelius Weig <[email protected]>
corneliusweig
force-pushed
the
w/default-config-name
branch
from
d854410 to
3693b15
Compare
|
@corneliusweig I actually don't think we need to edit our examples. |
As discussed in #2556
skaffold initcould populate themetadata.namefield inskaffold.yaml. This PR adds the logic to populate the field with the basename of the currect working dir. I thought about adding tests, but I decided against adding some, because the test-cases could only test the trivial happy path of the added function.Also, I have added the config name to all examples. This may be a controversial change..
Even though this field is optional in Skaffold, I think it makes the yaml files easier to navigate, because every example
skaffold.yamlstates what example it is.Close #2556