Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add container name validation during creation and push #3959

10 changes: 9 additions & 1 deletion pkg/odo/cli/component/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"github.com/openshift/odo/pkg/component"
"github.com/openshift/odo/pkg/config"
"github.com/openshift/odo/pkg/devfile"
adaptersCommon "github.com/openshift/odo/pkg/devfile/adapters/common"
"github.com/openshift/odo/pkg/devfile/parser"
"github.com/openshift/odo/pkg/devfile/parser/data/common"
"github.com/openshift/odo/pkg/envinfo"
Expand Down Expand Up @@ -835,13 +836,13 @@ func (co *CreateOptions) Validate() (err error) {
if err != nil {
return err
}

// Only validate namespace if pushtarget isn't docker
if !pushtarget.IsPushTargetDocker() {
err := util.ValidateK8sResourceName("component namespace", co.devfileMetadata.componentNamespace)
if err != nil {
return err
}

}

spinner.End(true)
Expand Down Expand Up @@ -1062,6 +1063,13 @@ func (co *CreateOptions) devfileRun() (err error) {
if err != nil {
return errors.Wrap(err, "unable to parse devfile")
}
containerComponents := adaptersCommon.GetDevfileContainerComponents(devObj.Data)
for _, comp := range containerComponents {
err := util.ValidateK8sResourceName("container name", comp.Name)
if err != nil {
return err
}
}

err = downloadStarterProject(devObj, co.devfileMetadata.starter, co.interactive)
if err != nil {
Expand Down
23 changes: 23 additions & 0 deletions pkg/odo/cli/component/push.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
ktemplates "k8s.io/kubectl/pkg/util/templates"

"github.com/openshift/odo/pkg/component"
adaptersCommon "github.com/openshift/odo/pkg/devfile/adapters/common"
"github.com/openshift/odo/pkg/devfile/parser"
"github.com/openshift/odo/pkg/log"
projectCmd "github.com/openshift/odo/pkg/odo/cli/project"
Expand Down Expand Up @@ -175,6 +176,28 @@ func (po *PushOptions) Validate() (err error) {
// If Devfile is present we do not need to validate the below S2I checks
// TODO: Perhaps one day move Devfile validation to here instead?
if util.CheckPathExists(po.DevfilePath) {
spinner := log.Spinner("Validating K8s Resources Name")
Copy link
Member

@johnmcollier johnmcollier Sep 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about just Validating devfile component as the spinner text, like odo create has?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

defer spinner.End(false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should validate under one spinner

$ odo push
 ✓  Validating devfile component [91679ns]

Validation
 ✓  Validating the devfile [22104ns]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

err = util.ValidateK8sResourceName("component name", po.EnvSpecificInfo.GetName())
if err != nil {
return err
}
containerComponents := adaptersCommon.GetDevfileContainerComponents(po.Devfile.Data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can extract https://github.com/openshift/odo/pull/3959/files#diff-6e3a2602ffd394e9a73d4fbf1388be8dR185-R190 out into a helper function and use it in create as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

	containerComponents := adaptersCommon.GetDevfileContainerComponents(devObj.Data)
	for _, comp := range containerComponents {
		err := util.ValidateK8sResourceName("container name", comp.Name)
		if err != nil {
			return err
		}
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated to use unit tests instead of integration tests

for _, comp := range containerComponents {
err := util.ValidateK8sResourceName("container name", comp.Name)
if err != nil {
return err
}
}
// Only validate namespace if pushtarget isn't docker
if !pushtarget.IsPushTargetDocker() {
err := util.ValidateK8sResourceName("component namespace", po.EnvSpecificInfo.GetNamespace())
if err != nil {
return err
}
}

spinner.End(true)
return nil
}

Expand Down
6 changes: 6 additions & 0 deletions tests/integration/devfile/cmd_devfile_create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,12 @@ var _ = Describe("odo devfile create command tests", func() {
It("should fail to create the devfile component with --devfile points to different devfile", func() {
helper.CmdShouldFail("odo", "create", "nodejs", "--devfile", "/path/to/file")
})

It("should fail to create the devfile component if the container name is too long", func() {
helper.ReplaceString("devfile.yaml", "runtime", "runtimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntime")
output := helper.CmdShouldFail("odo", "create", "--devfile", "./devfile.yaml")
Expect(output).Should(ContainSubstring("Contain at most 63 characters"))
})
})

Context("When devfile exists not in user's working directory and user specify the devfile path via --devfile", func() {
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/devfile/cmd_devfile_env_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
var _ = Describe("odo devfile env command tests", func() {
const (
testName = "testname"
testNamepace = "testNamepace"
testNamepace = "testnamepace"
testDebugPort = "8888"
fakeParameter = "fakeParameter"
)
Expand Down
11 changes: 11 additions & 0 deletions tests/integration/devfile/cmd_devfile_push_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -649,6 +649,17 @@ var _ = Describe("odo devfile push command tests", func() {

})

It("should fail to push the devfile component if the container name is too long", func() {

helper.CmdShouldPass("odo", "create", "nodejs", "--project", namespace, cmpName)

helper.CopyExample(filepath.Join("source", "devfiles", "nodejs", "project"), context)
helper.CopyExampleDevFile(filepath.Join("source", "devfiles", "nodejs", "devfile.yaml"), filepath.Join(context, "devfile.yaml"))
helper.ReplaceString("devfile.yaml", "runtime", "runtimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntime")
output := helper.CmdShouldFail("odo", "push", "--context", context)
Expect(output).Should(ContainSubstring("Contain at most 63 characters"))
})

})

Context("Verify files are correctly synced", func() {
Expand Down