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.

Sign up for GitHub

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

Jump to bottom

add container name validation during creation and push #3959

Merged
merged 11 commits into from
Oct 6, 2020
Merged

add container name validation during creation and push #3959

merged 11 commits into from
Oct 6, 2020

Conversation

yangcao77
Copy link
Contributor

@yangcao77 yangcao77 commented Sep 15, 2020 •
edited
Loading

Signed-off-by: Stephanie yangcao@redhat.com

What type of PR is this?
/kind bug
/area devfile

What does does this PR do / why we need it:
Add validation for devfile container name during component creation, and push. Block the action if the container name is not a valid K8s resourece name.

Which issue(s) this PR fixes:

Fixes #3698

PR acceptance criteria:

How to test changes / Special notes to the reviewer:

test component creation:
create a devfile with a long container name, and run odo create --devfile <devfile path>

$ odo create --devfile ./devfile.yaml 
Validation
 ✓  Creating a devfile component from devfile path: /Users/stephanie/Documents/devfile-spring2/devfile.yaml [62590ns]
 ✓  Validating devfile component [277583ns]
 ✗  endpoint name "runtimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntime" is not valid, endpoint name should conform the following requirements: 
- Contain at most 63 characters
- Contain only lowercase alphanumeric characters or ‘-’
- Start with an alphanumeric character
- End with an alphanumeric character
- Must not contain all numeric values

test odo push:
odo create nodejs --starter
modify the devfile to use a long container name
run odo push

$ odo push
 ✗  Validating K8s Resources Name [132166ns]
 ✗  endpoint name "runtimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntime" is not valid, endpoint name should conform the following requirements: 
- Contain at most 63 characters
- Contain only lowercase alphanumeric characters or ‘-’
- Start with an alphanumeric character
- End with an alphanumeric character
- Must not contain all numeric values

@yangcao77
add container name validation during creation and push
a9932b7 
Signed-off-by: Stephanie <yangcao@redhat.com>
@openshift-ci-robot openshift-ci-robot added kind/bug Categorizes issue or PR as related to a bug. area/devfile-spec Issues or PRs related to the Devfile specification and how odo handles and interprets it. labels Sep 15, 2020
@yangcao77
fix integration test failure
8870d8a 
Signed-off-by: Stephanie <yangcao@redhat.com>
@codecov
Copy link

codecov bot commented Sep 16, 2020 •
edited
Loading

Codecov Report

Merging #3959 into master will increase coverage by 0.01%.
The diff coverage is 35.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3959      +/-   ##
==========================================
+ Coverage   43.28%   43.30%   +0.01%     
==========================================
  Files         147      147              
  Lines       12412    12431      +19     
==========================================
+ Hits         5373     5383      +10     
- Misses       6471     6480       +9     
  Partials      568      568
Impacted Files Coverage Δ
...g/devfile/adapters/kubernetes/component/adapter.go 31.01% <0.00%> (-0.91%) ⬇️
pkg/devfile/validate/validate.go 21.73% <100.00%> (+21.73%) ⬆️
pkg/devfile/validate/commands.go 95.45% <0.00%> (+0.58%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eee175b...1327131. Read the comment docs.

@yangcao77
add integration tests
40011ab 
Signed-off-by: Stephanie <yangcao@redhat.com>
@girishramnani
Copy link
Contributor

@yangcao77 this seems like something that can be tested via unit test? So can we write a unit test instead of integration?

@yangcao77
Copy link
Contributor Author

@girishramnani The validation code added is mainly inside the cli modules, so IMO integration test is the better place to test for.
If we want to use unit test, I can only test the functionality of the util function ValidateK8sResourceName, not the actual cli behavior

johnmcollier
johnmcollier reviewed Sep 16, 2020
View reviewed changes
pkg/odo/cli/component/push.go Outdated
@@ -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 •
edited
Loading

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

@yangcao77
resolve conflict
98420af 
Signed-off-by: Stephanie <yangcao@redhat.com>
girishramnani
girishramnani reviewed Sep 17, 2020
View reviewed changes
pkg/odo/cli/component/push.go Outdated
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

@kadel
Copy link
Member

kadel commented Sep 17, 2020

/approve

@kadel
Copy link
Member

kadel commented Sep 17, 2020

/retest

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Sep 17, 2020
@yangcao77
use unit test instead of integration test
aafdfaa 
Signed-off-by: Stephanie <yangcao@redhat.com>
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Sep 21, 2020
@yangcao77
rebase
ac976a8 
Signed-off-by: Stephanie <yangcao@redhat.com>
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Sep 21, 2020
@yangcao77
fix integration test failure
f92f9cb 
Signed-off-by: Stephanie <yangcao@redhat.com>
@yangcao77
Copy link
Contributor Author

/retest

maysunfaisal
maysunfaisal reviewed Sep 21, 2020
View reviewed changes
pkg/devfile/validate/validate.go Outdated
@@ -44,3 +47,15 @@ func ValidateDevfileData(data interface{}) error {
return nil

}

// ValidateContatinerName validates whether the container name is valid for K8
func ValidateContatinerName(devfileData data.DevfileData) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

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

@yangcao77
Merge branch 'master' of github.com:openshift/odo into 3968-container…
60ea264 
…NameToolong
@yangcao77
fix typo
f68b19e 
Signed-off-by: Stephanie <yangcao@redhat.com>
adisky
adisky suggested changes Sep 23, 2020
View reviewed changes
pkg/odo/cli/component/push.go Outdated
@@ -175,6 +176,25 @@ 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 devfile component")
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

@cdrage
Copy link
Member

cdrage commented Oct 6, 2020

/lgtm

Reviewed and looks good! Thanks @yangcao77

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Oct 6, 2020
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

5 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 19098a2 into redhat-developer:master Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maysunfaisal maysunfaisal maysunfaisal left review comments

@johnmcollier johnmcollier johnmcollier left review comments

@girishramnani girishramnani girishramnani left review comments

@adisky adisky adisky requested changes

@cdrage cdrage Awaiting requested review from cdrage

Assignees

@cdrage cdrage

@adisky adisky

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. area/devfile-spec Issues or PRs related to the Devfile specification and how odo handles and interprets it. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Odo fails when devfile component container name is really long
10 participants
@yangcao77 @girishramnani @kadel @openshift-ci-robot @cdrage @openshift-bot @johnmcollier @adisky @maysunfaisal @openshift-merge-robot