Implement Starter Projects for devfile #3673

adisky · 2020-07-31T13:01:35Z

What type of PR is this?

/kind feature

What does does this PR do / why we need it:
Implement Starter Project, and also interactive mode for the same

Which issue(s) this PR fixes:

PR acceptance criteria:

Unit test (Updated project tests for starter project)
Integration test (Tests were already added for --starter flag)
Documentation

How to test changes / Special notes to the reviewer:
odo create --starter, should download starter project from devfile
odo create, In interactive mode should ask to download starter projects.

adisky · 2020-07-31T13:05:40Z

Not updated sync mechanism and some project references as this PR #3662 already doing that

adisky · 2020-08-06T10:10:55Z

failures

Summarizing 4 Failures:
[Fail] odo devfile create command tests When executing odo create with devfile component and --starter flag [It] should successfully create the component and download the source 
/home/travis/gopath/src/github.com/openshift/odo/tests/helper/helper_run.go:34
[Fail] odo devfile create command tests When executing odo create using --starter with a devfile component that contains no projects [It] should fail with please run 'No project found in devfile component.' 
/home/travis/gopath/src/github.com/openshift/odo/tests/helper/helper_generic.go:67
[Fail] odo devfile create command tests When executing odo create with devfile component and --starter flag [It] should successfully create the component specified with valid project and download the source 
/home/travis/gopath/src/github.com/openshift/odo/tests/helper/helper_run.go:34
[Fail] odo devfile create command tests When executing odo create with an invalid project specified in --starter [It] should fail with please run 'The project: invalid-project-name specified in --starter does not exist' 
/home/travis/gopath/src/github.com/openshift/odo/tests/helper/helper_generic.go:67

failing as all registry devfile using projects, These dates would pass with this PR odo-devfiles/registry#29

kadel · 2020-08-06T11:04:05Z

/retest

adisky · 2020-08-06T13:51:27Z

@kadel tests would fail, until this PR odo-devfiles/registry#29 gets merged

cdrage · 2020-08-06T18:50:11Z

pkg/devfile/parser/data/1.0.0/components.go

+	return nil
+}
+
+func (d *Devfile100) AddStarterProjects(projects []common.DevfileStarterProject) error { return nil }


Missing comments

it is devfile V1 package, we are just adding functions to satisfy interface, We are not using them and eventually be removed.

cdrage · 2020-08-06T18:50:15Z

pkg/devfile/parser/data/1.0.0/components.go

+
+func (d *Devfile100) AddStarterProjects(projects []common.DevfileStarterProject) error { return nil }
+
+func (d *Devfile100) UpdateStarterProject(project common.DevfileStarterProject) {}


Missing comments

pkg/devfile/parser/data/2.0.0/components.go

cdrage · 2020-08-06T18:53:04Z

pkg/odo/cli/component/create.go

@@ -895,8 +887,8 @@ func (co *CreateOptions) downloadProject(projectPassed string) error {
 		return errors.Errorf("Project type not supported")
 	}

-	log.Info("\nProject")
-	downloadSpinner := log.Spinnerf("Downloading project from %s", logUrl)
+	log.Info("\nStarter Project")


I think we should keep this as just Project not Starter Project. Just my opinion though! I think it would fit the output a bit better.

I guess it might confuse users with projects.

Okay, we'll keep it as Starter Project. It looks kind of out of place, but that's just me. All below output seems to be starter project related only, so makes sense.

pkg/odo/cli/component/create.go

mik-dass · 2020-08-10T11:36:07Z

pkg/devfile/parser/types.go

+// OverrideStarterProjects overrides the projects of the parent devfile
+// overridePatch contains the patches to be applied to the parent's projects


starterProjects

mik-dass · 2020-08-10T11:38:12Z

pkg/devfile/parser/types.go

+			}
+		}
+		if !found {
+			return fmt.Errorf("the command to override is not found in the parent")


the starterProject to override

pkg/odo/cli/component/create.go

pkg/devfile/parser/data/2.0.0/components.go

mik-dass · 2020-08-10T11:45:53Z

pkg/devfile/parser/data/2.0.0/components.go

+	return d.StarterProjects
+}
+
+// AddStarterProjects adds the slice of Devfile projects to the Devfile's project list


Devfile's starterProjects

mik-dass · 2020-08-10T11:46:09Z

pkg/devfile/parser/data/2.0.0/components.go

+	return nil
+}
+
+// UpdateStarterProject updates the slice of DevfileCommand projects parsed from the Devfile


Devfile starterProjects

adisky · 2020-08-17T07:55:13Z

/retest

020/08/13 18:09:30 No custom metadata found and prow metadata already exists. Not updating the metadata.
2020/08/13 18:09:30 Ran for 3h13m23s
error: some steps failed:
  * could not run steps: step integration-e2e failed: "integration-e2e" post steps failed: "integration-e2e" pod "integration-e2e-gather-extra" failed: the pod ci-op-vq41mrtf/integration-e2e-gather-extra failed after 49s (failed containers: test): ContainerFailed one or more containers exited

adisky · 2020-08-17T11:22:34Z

/retest

• Failure [90.392 seconds]
odo source e2e tests
/go/src/github.com/openshift/odo/tests/e2escenarios/e2e_source_test.go:14
  odo component creation from source
  /go/src/github.com/openshift/odo/tests/e2escenarios/e2e_source_test.go:33
    Should be able to deploy a wildfly source application [It]
    /go/src/github.com/openshift/odo/tests/e2escenarios/e2e_source_test.go:40
    Failed after 30 retries. Content in http://wildfly-app-8080-app-lrfuzcojpd.apps.ci-op-ih2xf9z5-2f74e.origin-ci-int-aws.dev.rhcloud.com doesn't include 'Insult'.

adisky · 2020-08-17T11:52:32Z

@kadel tests would fail, until this PR odo-devfiles/registry#29 gets merged

updates tests to use devfile from odo repo /tests/examples/devfile

mik-dass · 2020-08-17T12:26:30Z

pkg/devfile/parser/types.go

+
+// OverrideStarterProjects overrides the starter projects of the parent devfile
+// overridePatch contains the patches to be applied to the parent's starter projects
+func (d DevfileObj) OverrideStarterProjects(overridePatch []common.DevfileStarterProject) error {


unit test for this function

mik-dass · 2020-08-17T12:36:19Z

pkg/odo/cli/component/ui/ui.go

+
+	var download = false
+	var selectedProject string
+	prompt := &survey.Confirm{Message: "Do you want to download starter project"}


download a starter project

mik-dass · 2020-08-17T13:00:57Z

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

+type DevfileStarterProject struct {
+
+	// Project name
+	Name string `json:"name"`


Please add yaml tags for these fields

johnmcollier · 2020-08-17T13:45:10Z

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

@@ -275,6 +275,9 @@ type DevfileParent struct {

 	// Bindings of commands to events. Each command is referred-to by its name.
 	Events DevfileEvents `json:"events,omitempty" yaml:"events,omitempty"`
+
+	// StarterProjects is a project that can be used as a starting point when bootstrapping new projects
+	StarterProjects []DevfileStarterProject `json:"starterProjects,omitempty" yaml:"starterProjects,omitempty"`


Curious, why do we add the StarterProjects type to the common package in the parser, rather than under parser/data/2.0.0?

every devfile struct(1.0, 2.0) needs to satisfy this interface https://github.com/openshift/odo/blob/master/pkg/devfile/parser/data/interface.go#L8
the methods here used everywhere in adapters code, to make the adapters code work on a common type regardless of devfile version passed, it has been done in this way.

cdrage

You'll need to update:

https://github.com/openshift/odo/blob/master/docs/file-reference/index.md

As well since we're doing a considerable change to the Devfile file reference.

cdrage · 2020-08-17T15:24:41Z

pkg/devfile/parser/types.go

+
+				err = json.Unmarshal(merged, &updatedProject)
+				if err != nil {
+					return err


I think you should add a description to append to the error using errors.Wrap(err, "unable to override starter project cannot unmarshal for example. Since we use json.Unmarshal in other parts of the Devfile code when unmarshaling and it may be difficult to determine where the error is coming from.

@cdrage json.Unmarshal specifies the field where the error is coming from, I think the returned error gives sufficient information about the fields.
https://github.com/golang/go/blob/master/src/encoding/json/decode.go#L134

e.g.
https://play.golang.org/p/uQUjwN3H35G

json: cannot unmarshal string into Go struct field starterProject.ProjectNo of type int

Yes, but you should ideally wrap it as well since we use json.Unmarshal in other sections of our code. So having a clear indicator that it's coming from unmarshaling the starter project makes it easier to find where the error is coming from...

Thanks, Done.

cdrage · 2020-08-17T15:27:23Z

pkg/devfile/parser/types_test.go

+		wantErr        bool
+	}{
+		{
+			name: "case 1: override a starter project's fields",


nitpick, please capitalize case 1, etc.

override a starter projects field

cdrage · 2020-08-17T15:30:56Z

pkg/odo/cli/component/create.go

@@ -895,8 +887,8 @@ func (co *CreateOptions) downloadProject(projectPassed string) error {
 		return errors.Errorf("Project type not supported")
 	}

-	log.Info("\nProject")
-	downloadSpinner := log.Spinnerf("Downloading project from %s", logUrl)
+	log.Info("\nStarter Project")


Okay, we'll keep it as Starter Project. It looks kind of out of place, but that's just me. All below output seems to be starter project related only, so makes sense.

codecov · 2020-08-17T16:37:17Z

Codecov Report

Merging #3673 into master will increase coverage by 0.04%.
The diff coverage is 54.16%.

@@            Coverage Diff             @@
##           master    #3673      +/-   ##
==========================================
+ Coverage   44.15%   44.19%   +0.04%     
==========================================
  Files         139      139              
  Lines       13345    13388      +43     
==========================================
+ Hits         5892     5917      +25     
- Misses       6874     6888      +14     
- Partials      579      583       +4

Impacted Files	Coverage Δ
pkg/testingutil/devfile.go	`0.00% <0.00%> (ø)`
pkg/devfile/parser/parse.go	`53.84% <33.33%> (-2.09%)`	⬇️
pkg/devfile/parser/data/2.0.0/components.go	`39.03% <62.50%> (+2.19%)`	⬆️
pkg/devfile/parser/types.go	`70.21% <63.63%> (+1.38%)`	⬆️

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 7d1a9c1...4bedef6. Read the comment docs.

adisky · 2020-08-18T11:17:56Z

@cdrage opened a WIP PR for updating documentation
#3770
Need to discuss few things on documentation part.

cdrage

New changes LGTM! Thanks @adisky

/approve

openshift-ci-robot · 2020-08-19T15:44:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdrage

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:

~~OWNERS~~ [cdrage]

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 requested review from GeekArthur and johnmcollier July 31, 2020 13:02

adisky force-pushed the starter-project-final branch from 2d04d9f to 2bd9234 Compare August 4, 2020 10:34

adisky mentioned this pull request Aug 6, 2020

Update devfile with starter projects odo-devfiles/registry#29

Merged

adisky assigned mik-dass and GeekArthur Aug 6, 2020

adisky requested a review from mik-dass August 6, 2020 10:19

adisky assigned johnmcollier Aug 6, 2020

cdrage suggested changes Aug 6, 2020

View reviewed changes

openshift-ci-robot assigned cdrage Aug 6, 2020

adisky force-pushed the starter-project-final branch from 2bd9234 to 9966842 Compare August 7, 2020 10:46

mik-dass reviewed Aug 10, 2020

View reviewed changes

pkg/odo/cli/component/create.go Show resolved Hide resolved

mik-dass reviewed Aug 10, 2020

View reviewed changes

pkg/odo/cli/component/create.go Show resolved Hide resolved

mik-dass reviewed Aug 10, 2020

View reviewed changes

pkg/devfile/parser/data/2.0.0/components.go Show resolved Hide resolved

mik-dass reviewed Aug 10, 2020

View reviewed changes

adisky force-pushed the starter-project-final branch from cf44d2c to 358bfca Compare August 13, 2020 14:55

mik-dass reviewed Aug 17, 2020

View reviewed changes

johnmcollier reviewed Aug 17, 2020

View reviewed changes

cdrage suggested changes Aug 17, 2020

View reviewed changes

Aditi Sharma added 10 commits August 18, 2020 15:44

Implement Starter Projects

47baa15

Update Devfile to starterProjects

fbcd34a

fix gosec

d9c41f8

Address review comments

53e1f4f

Address review comments

a6dd89d

Address review comments2

21de897

fix integration tests

2844459

Address review comments

a2ab1ff

Add yaml tag to starter project

71064a4

Address Charlie's review comments

f0fceab

adisky force-pushed the starter-project-final branch from 6be314c to f0fceab Compare August 18, 2020 10:18

Wrap error while json.unmarshal

4bedef6

cdrage approved these changes Aug 19, 2020

View reviewed changes

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

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Aug 19, 2020

openshift-merge-robot merged commit 17ab740 into redhat-developer:master Aug 19, 2020


		func (d *Devfile100) AddStarterProjects(projects []common.DevfileStarterProject) error { return nil }

		func (d *Devfile100) UpdateStarterProject(project common.DevfileStarterProject) {}

		// OverrideStarterProjects overrides the projects of the parent devfile
		// overridePatch contains the patches to be applied to the parent's projects

Implement Starter Projects for devfile #3673

Implement Starter Projects for devfile #3673

Conversation

adisky commented Jul 31, 2020 • edited Loading

adisky commented Jul 31, 2020

adisky commented Aug 6, 2020

kadel commented Aug 6, 2020

adisky commented Aug 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

mik-dass Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

adisky commented Aug 17, 2020

adisky commented Aug 17, 2020

adisky commented Aug 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisky Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 17, 2020 • edited Loading

Codecov Report

adisky commented Aug 18, 2020

cdrage left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 19, 2020

adisky commented Jul 31, 2020 •

edited

Loading

mik-dass Aug 10, 2020 •

edited

Loading

mik-dass Aug 10, 2020 •

edited

Loading

adisky Aug 18, 2020 •

edited

Loading

codecov bot commented Aug 17, 2020 •

edited

Loading