Updates name and id field in the devfile schema by mik-dass · Pull Request #3872 · redhat-developer/odo

mik-dass · 2020-08-31T15:52:53Z

What type of PR is this?

/kind api-change

What does does this PR do / why we need it:

Updates the schema and changes the code according to it.

Which issue(s) this PR fixes:

Fixes #3822

PR acceptance criteria:

Unit test
Integration test
Documentation
I have read the test guidelines

How to test changes / Special notes to the reviewer:

Tests should pass.

openshift-ci-robot · 2020-08-31T15:53:10Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

adisky · 2020-09-09T08:46:15Z

pkg/devfile/adapters/common/command.go

-func validateCompositeCommand(data data.DevfileData, compositeCommand *common.Composite, parentCommands map[string]string, devfileCommands map[string]common.DevfileCommand) error {
-	if compositeCommand.Group != nil && compositeCommand.Group.Kind == common.RunCommandGroupType {
+func validateCompositeCommand(data data.DevfileData, compositeCommand *common.DevfileCommand, parentCommands map[string]string, devfileCommands map[string]common.DevfileCommand) error {
+	if compositeCommand.Composite.Group != nil && compositeCommand.Composite.Group.Kind == common.RunCommandGroupType {


not important currently, compositeCommand.Composite.Group looking repetitive, parameter compositeCommand could be renamed to command.

OK let's handle this in a separate issue.

pkg/devfile/parser/data/common/command_helper.go

kadel · 2020-09-09T14:48:37Z

/approve

kadel · 2020-09-09T16:23:23Z

/hold cancel

maysunfaisal · 2020-09-09T16:34:34Z

pkg/devfile/adapters/common/command.go

 	isComponentValid := false
 	for _, component := range components {
-		if command.Exec.Component == component.Container.Name {
+		if command.Exec.Component == component.Name {


I'm sorry, I was just glancing through. But does this mean the above statement is true if i have a volume component whose name matches the command exec component?

Because i was working on a clean up PR #3912 that was touching these files

@maysunfaisal Because of the new format for component names, I don't believe it's valid any more to have two components of different types, but with the same name.

@maysunfaisal As mentioned by @johnmcollier, I don't think it will be possible with the new schema.

No i meant what if I have an invalid devfile like this:

components: - name: tools container: image: maysunfaisal/springbootbuild memoryLimit: 768Mi command: ['tail'] args: [ '-f', '/dev/null'] volumeMounts: - name: springbootpvc path: /data mountSources: true - name: springbootpvc volume: {} commands: - id: defaultBuild exec: component: springbootpvc commandLine: "/artifacts/bin/build-container-full.sh" workingDir: /projects group: kind: build isDefault: true

but i realized that component is coming from components := GetDevfileContainerComponents(data), so we're good :)

johnmcollier · 2020-09-09T17:07:16Z

pkg/devfile/parser/data/common/command_helper.go

@@ -4,10 +4,8 @@ import "strings"

 // GetID returns the ID of the command
 func (dc DevfileCommand) GetID() string {


Wondering if we even need GetID anymore? It was mostly needed when the ID wasn't stored at the top level part of the command and we had to check either Composite or Exec to get it.

But TBH, we can remove in a later PR. It's fine to leave in now.

Yeah, let's remove that in a later PR

I think we can keep it in, its always neat to have helper funcs even if we can get it directly now :)

cdrage · 2020-09-09T17:46:47Z

Just so everyone else knows, this explains the errors I've been getting:

[odo]  ✗  invalid devfile schema. errors :
[odo] - commands.0: Additional property id is not allowed
[odo] - commands.0.exec: id is required
[odo] - commands.1: Additional property id is not allowed
[odo] - commands.1.exec: id is required
[odo] - commands.2: Additional property id is not allowed
[odo] - commands.2.exec: id is required
[odo] - commands.3: Additional property id is not allowed
[odo] - commands.3.exec: id is required
[odo] - components.0: Additional property name is not allowed
[odo] - components.0.container: name is required                                                                                                                                                                                                                                 
[odo]

cdrage · 2020-09-09T17:48:13Z

/approve

Went through the code and all of this looks good to me.

Tests pass.

Lets get this merged!

openshift-ci-robot · 2020-09-09T17:48:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdrage, kadel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [cdrage,kadel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

johnmcollier

Looked through changes, all looks fine to me, don't see anything glaring that couldn't wait for another PR later. Also tested manually seems to work fine.

Let's get this one in if there's no objections.

/lgtm

codecov · 2020-09-09T19:32:50Z

Codecov Report

Merging #3872 into master will increase coverage by 0.18%.
The diff coverage is 61.08%.

@@            Coverage Diff             @@
##           master    #3872      +/-   ##
==========================================
+ Coverage   44.60%   44.78%   +0.18%     
==========================================
  Files         143      143              
  Lines       13860    13911      +51     
==========================================
+ Hits         6182     6230      +48     
  Misses       7089     7089              
- Partials      589      592       +3

Impacted Files	Coverage Δ
pkg/component/component.go	`25.76% <0.00%> (ø)`
pkg/devfile/parser/representation.go	`0.00% <0.00%> (ø)`
pkg/envinfo/envinfo.go	`43.93% <0.00%> (-0.52%)`	⬇️
pkg/odo/cli/storage/create.go	`0.00% <0.00%> (ø)`
pkg/odo/cli/utils/convert.go	`0.00% <0.00%> (ø)`
pkg/testingutil/devfile.go	`0.00% <0.00%> (ø)`
...g/devfile/adapters/kubernetes/component/adapter.go	`32.93% <9.09%> (-0.41%)`	⬇️
pkg/devfile/parser/data/common/command_helper.go	`81.81% <40.00%> (+5.50%)`	⬆️
pkg/devfile/adapters/docker/component/utils.go	`69.66% <46.15%> (ø)`
pkg/devfile/parser/data/2.0.0/components.go	`38.37% <50.00%> (-0.66%)`	⬇️
... and 10 more

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 7831c92...84637af. Read the comment docs.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/api-change labels Aug 31, 2020

openshift-ci-robot requested review from adisky and girishramnani August 31, 2020 15:53

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 6, 2020

mik-dass force-pushed the name_update branch from e3bbcc2 to 5ed43e6 Compare September 7, 2020 10:40

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 7, 2020

mik-dass force-pushed the name_update branch from 5ed43e6 to 7bf7c50 Compare September 9, 2020 05:12

mik-dass marked this pull request as ready for review September 9, 2020 05:13

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Sep 9, 2020

mik-dass added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Sep 9, 2020

mik-dass force-pushed the name_update branch 4 times, most recently from f85b4cc to ec7ae47 Compare September 9, 2020 07:56

adisky reviewed Sep 9, 2020

View reviewed changes

mik-dass force-pushed the name_update branch 3 times, most recently from 2529968 to de7d6bc Compare September 9, 2020 12:02

Updates name and id field in the devfile schema

309677f

mik-dass force-pushed the name_update branch from de7d6bc to 309677f Compare September 9, 2020 14:37

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 9, 2020

Reverts back to the default github registry

84637af

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Sep 9, 2020

maysunfaisal reviewed Sep 9, 2020

View reviewed changes

johnmcollier reviewed Sep 9, 2020

View reviewed changes

johnmcollier approved these changes Sep 9, 2020

View reviewed changes

openshift-ci-robot assigned johnmcollier Sep 9, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 9, 2020

GeekArthur mentioned this pull request Sep 9, 2020

Secure starter project support #3743

Merged

2 tasks

openshift-merge-robot merged commit 8c9fbdb into redhat-developer:master Sep 9, 2020

This was referenced Sep 9, 2020

Update Devfiles and odo to work with breaking changes in devfile spec #3822

Closed

Update docs according to schema changes in the command and component struct #3925

Closed

johnmcollier mentioned this pull request Sep 9, 2020

Implement clonePath, update source code sync location #3907

Merged

4 tasks

mik-dass deleted the name_update branch September 10, 2020 07:10

		@@ -4,10 +4,8 @@ import "strings"

		// GetID returns the ID of the command
		func (dc DevfileCommand) GetID() string {

Conversation

mik-dass commented Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Aug 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kadel commented Sep 9, 2020

Uh oh!

kadel commented Sep 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maysunfaisal Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdrage commented Sep 9, 2020

Uh oh!

cdrage commented Sep 9, 2020

Uh oh!

openshift-ci-robot commented Sep 9, 2020

Uh oh!

johnmcollier left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 9, 2020

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mik-dass commented Aug 31, 2020 •

edited

Loading

maysunfaisal Sep 9, 2020 •

edited

Loading