Fix: include starter projects for $PROJECT_SOURCE path selection #1190

AObuchow · 2023-10-13T16:38:09Z

What does this PR do?

This PR adds all starterProjects to the list of projects used when determining the value of the $PROJECT_SOURCE environment variable used in workspace deployments.

Now, when determining the value of $PROJECT_SOURCE, the following rules are considered (in order of priority):

If the workspace has at least one regular project, the first one will be selected. If the first project has a clone path, it will be used, otherwise the project's name will be used for the project source path. This was the default behaviour prior to this PR.
If the workspace has a starter project that is selected using the controller.devfile.io/use-starter-project workspace attribute, its name will be used for project source path.
Otherwise, the returned project source path will be an empty string, i.e. $PROJECT_SOURCE = /projects/

I also removed a completed TODO comment about adding sparseCheckout support to the project clone container and moved the GetClonePath() function to be accessible from other packages.

What issues does this PR fix or reference?

#1189

Is it tested? How?

There are 4 cases to test, each is described below.

devworkspace with only starter projects

Apply a devworkspace that has at least one starter project, and no regular projects, such as the following:

kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: plain-starter-projects
spec:
  started: true
  routingClass: 'basic'
  template:
    starterProjects:
    - name: community
      zip:
        location: https://code.quarkus.io/d?e=io.quarkus%3Aquarkus-resteasy&e=io.quarkus%3Aquarkus-micrometer&e=io.quarkus%3Aquarkus-smallrye-health&e=io.quarkus%3Aquarkus-openshift&cn=devfile
    - name: redhat-product
      zip:
        location: https://code.quarkus.redhat.com/d?e=io.quarkus%3Aquarkus-resteasy&e=io.quarkus%3Aquarkus-smallrye-health&e=io.quarkus%3Aquarkus-openshift
    components:
      - name: web-terminal
        container:
          image: quay.io/wto/web-terminal-tooling:next
          memoryRequest: 256Mi
          memoryLimit: 512Mi
          mountSources: true
          command:
           - "tail"
           - "-f"
           - "/dev/null"

Ensure the workspace pod's spec has the $PROJECT_SOURCE environment variable set to /projects/ as no starter project has been selected. For the previously given devworkspace, we should have:

spec:
(...)
  containers:
    - name: web-terminal
      env:
        - name: PROJECTS_ROOT
          value: /projects
        - name: PROJECT_SOURCE
          value: /projects/
(...)

devworkspace with starter projects and regular projects

Apply a devworkspace that has a regular project as well as starter projects. For example:

kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: plain-starter-projects
spec:
  started: true
  routingClass: 'basic'
  template:
    starterProjects:
    - name: community
      zip:
        location: https://code.quarkus.io/d?e=io.quarkus%3Aquarkus-resteasy&e=io.quarkus%3Aquarkus-micrometer&e=io.quarkus%3Aquarkus-smallrye-health&e=io.quarkus%3Aquarkus-openshift&cn=devfile
    - name: redhat-product
      zip:
        location: https://code.quarkus.redhat.com/d?e=io.quarkus%3Aquarkus-resteasy&e=io.quarkus%3Aquarkus-smallrye-health&e=io.quarkus%3Aquarkus-openshift
    projects:
      - name: web-nodejs-sample
        git:
          remotes:
            origin: "https://github.com/che-samples/web-nodejs-sample.git"
    components:
      - name: web-terminal
        container:
          image: quay.io/wto/web-terminal-tooling:next
          memoryRequest: 256Mi
          memoryLimit: 512Mi
          mountSources: true
          command:
           - "tail"
           - "-f"
           - "/dev/null"

Ensure the workspace pod's spec has the $PROJECT_SOURCE environment variable set to /projects/<first-project-name>. For the previously given devworkspace, we should have:

spec:
(...)
  containers:
    - name: web-terminal
      env:
        - name: PROJECTS_ROOT
          value: /projects
        - name: PROJECT_SOURCE
          value: /projects/web-nodejs-sample
(...)

devworkspace with only starter projects, with a starter project being selected

Apply a devworkspace that has at least two starter projects, where one of the starter projects is selected using the controller.devfile.io/use-starter-project attribute, such as the following:

kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: plain-starter-projects
spec:
  started: true
  routingClass: 'basic'
  template:
    attributes:
      controller.devfile.io/use-starter-project: redhat-product
    starterProjects:
    - name: community
      zip:
        location: https://code.quarkus.io/d?e=io.quarkus%3Aquarkus-resteasy&e=io.quarkus%3Aquarkus-micrometer&e=io.quarkus%3Aquarkus-smallrye-health&e=io.quarkus%3Aquarkus-openshift&cn=devfile
    - name: redhat-product
      zip:
        location: https://code.quarkus.redhat.com/d?e=io.quarkus%3Aquarkus-resteasy&e=io.quarkus%3Aquarkus-smallrye-health&e=io.quarkus%3Aquarkus-openshift
    components:
      - name: web-terminal
        container:
          image: quay.io/wto/web-terminal-tooling:next
          memoryRequest: 256Mi
          memoryLimit: 512Mi
          mountSources: true
          command:
           - "tail"
           - "-f"
           - "/dev/null"

Ensure the workspace pod's spec has the $PROJECT_SOURCE environment variable set to the starter project that was selected. For the previously given devworkspace, we should have:

spec:
(...)
  containers:
    - name: web-terminal
      env:
        - name: PROJECTS_ROOT
          value: /projects
        - name: PROJECT_SOURCE
          value: /projects/redhat-product
(...)

devworkspace with regular and starter projects, with a selected starter project

Apply a devworkspace that has a regular and starter project, and the starter project is selected using the controller.devfile.io/use-starter-project attribute, such as the following:

kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: plain-starter-projects
spec:
  started: true
  routingClass: 'basic'
  template:
    attributes:
      controller.devfile.io/use-starter-project: redhat-product
    starterProjects:
    - name: community
      zip:
        location: https://code.quarkus.io/d?e=io.quarkus%3Aquarkus-resteasy&e=io.quarkus%3Aquarkus-micrometer&e=io.quarkus%3Aquarkus-smallrye-health&e=io.quarkus%3Aquarkus-openshift&cn=devfile
    - name: redhat-product
      zip:
        location: https://code.quarkus.redhat.com/d?e=io.quarkus%3Aquarkus-resteasy&e=io.quarkus%3Aquarkus-smallrye-health&e=io.quarkus%3Aquarkus-openshift
    projects:
      - name: web-nodejs-sample
        git:
          remotes:
            origin: "https://github.com/che-samples/web-nodejs-sample.git"
    components:
      - name: web-terminal
        container:
          image: quay.io/wto/web-terminal-tooling:next
          memoryRequest: 256Mi
          memoryLimit: 512Mi
          mountSources: true
          command:
           - "tail"
           - "-f"
           - "/dev/null"

Ensure the workspace pod's spec has the $PROJECT_SOURCE environment variable set to /projects/<first-project-name>. The regular project takes priority over the selected starter project when setting $PROJECT_SOURCE. For the previously given devworkspace, we should have:

spec:
(...)
  containers:
    - name: web-terminal
      env:
        - name: PROJECTS_ROOT
          value: /projects
        - name: PROJECT_SOURCE
          value: /projects/web-nodejs-sample
(...)

PR Checklist

E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
- v8-devworkspace-operator-e2e: DevWorkspace e2e test
- v8-che-happy-path: Happy path for verification integration with Che

codecov · 2023-10-13T17:30:53Z

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (b12f682) 52.90% compared to head (f13b5fe) 52.98%.
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1190      +/-   ##
==========================================
+ Coverage   52.90%   52.98%   +0.08%     
==========================================
  Files          84       84              
  Lines        7551     7583      +32     
==========================================
+ Hits         3995     4018      +23     
- Misses       3273     3276       +3     
- Partials      283      289       +6

Files	Coverage Δ
pkg/library/container/container.go	`38.77% <0.00%> (-7.90%)`	⬇️
pkg/library/container/mountSources.go	`77.61% <60.00%> (-12.22%)`	⬇️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

amisevsk · 2023-10-13T20:48:15Z

pkg/library/projects/util.go

+// Returns all projects (including starter projects) present in a DevWorkspaceTemplateSpec
+func GetAllProjects(workspace *dw.DevWorkspaceTemplateSpec) []dw.Project {
+	projects := workspace.Projects
+
+	// Note: starter project does not allow for specifying clonePath
+	for _, starterProject := range workspace.StarterProjects {
+
+		project := dw.Project{
+			Name:          starterProject.Name,
+			Attributes:    starterProject.Attributes,
+			ProjectSource: starterProject.ProjectSource,
+		}
+		projects = append(projects, project)
+	}
+	return projects
+}


I'm not sure this is correct behavior, in-context (haven't had a chance to verify though).

If a resolved workspace ends up with e.g. no projects and two starterProjects, with the second one being the "selected" project, how does the caller know that e.g. the first project will not be cloned but the second one will?

You're correct - there's a conflict with the "selected" starter project and how getProjectSourcePath() only selects the first available project to set $PROJECT_SOURCE.

I wasn't sure if this rule of "the first available project is what $PROJECT_SOURCE maps to" was documented somewhere, and I couldn't find it, or if it was just a decision that made sense at the time.

I'll rework things to respect the selected starter project attribute.

The env var is (poorly) documented as part of the Devfile schema, in .commands.exec.commandline:

$PROJECT_SOURCE: A path to a project source ($PROJECTS_ROOT/). If there are multiple projects, this will point to the directory of the first one.

Extending this to starterProjects, maybe it makes sense that it's the first project, with starterProjects coming after all regular projects?

Thank you for clarifying on the documentation of $PROJECT_SOURCE @amisevsk 🙏

@l0rd @ibuziuk do you have any input on this? For context:
eclipse-che/che#22601 specifies that the value of $PROJECT_SOURCE should point to the first starterProject. This PR resolves that requirement.

However, with #1130, you can now use the controller.devfile.io/use-starter-project attribute in a devworkspace to select which starterProject should be cloned.

With the current implementation of my PR, you could encounter a bug where $PROJECT_SOURCE is set to a starterProject that was not cloned.

E.g. given the following devworkspace, $PROJECT_SOURCE will be set to /projects/community but redhat-product will be cloned:

kind: DevWorkspace apiVersion: workspace.devfile.io/v1alpha2 metadata: name: plain-starter-projects spec: started: true routingClass: 'basic' template: attributes: controller.devfile.io/use-starter-project: redhat-product starterProjects: - name: community zip: location: https://code.quarkus.io/d?e=io.quarkus%3Aquarkus-resteasy&e=io.quarkus%3Aquarkus-micrometer&e=io.quarkus%3Aquarkus-smallrye-health&e=io.quarkus%3Aquarkus-openshift&cn=devfile - name: redhat-product zip: location: https://code.quarkus.redhat.com/d?e=io.quarkus%3Aquarkus-resteasy&e=io.quarkus%3Aquarkus-smallrye-health&e=io.quarkus%3Aquarkus-openshift components: - name: web-terminal container: image: quay.io/wto/web-terminal-tooling:next memoryRequest: 256Mi memoryLimit: 512Mi mountSources: true command: - "tail" - "-f" - "/dev/null"

I could try and rework this PR to fix this conflict with the use-starter-project attribute, and merge it if it is ready fast enough for DWO 0.24.0. Or we can merge it as-is and make a followup issue/PR to address this bug.

Also AFAIK, we are currently not allowing the user to select a starter project in Che's UI yet.

If the attribute controller.devfile.io/use-starter-project is set, then we should configure PROJECT_SOURCE accordingly, otherwise we should fall back on the first project.

And Eclipse Che dashboard is currently setting the attribute at startup (without interaction).

AObuchow · 2023-10-16T19:53:13Z

@amisevsk I've added 2 extra commits to address the cases where the controller.devfile.io/use-starter-project attribute is used (however, the second fixup commit was just some code style changes that are optional).

I also updated the PR description, including 2 new test cases.

The commit description for 755dcd8 (#1190) needs to be updated before this PR is merged to mention the new rules used when determining the value of $PROJECT_SOURCE.

amisevsk

A few comments -- I think we're going down the path of patching existing code to do something different when rewriting the logic makes things clearer.

amisevsk · 2023-10-17T16:21:28Z

pkg/library/container/container.go

+		err = handleMountSources(k8sContainer, component.Container, workspace)
+		if err != nil {
+			return nil, err
+		}


(Optional, feel free to just resolve)

Suggested change

err = handleMountSources(k8sContainer, component.Container, workspace)

if err != nil {

return nil, err

}

if err := handleMountSources(k8sContainer, component.Container, workspace); err != nil {

return nil, err

}

I always think it's a little weird that Go code goes around reassigning err over and over again

amisevsk · 2023-10-17T16:23:31Z

pkg/library/container/mountSources.go

+//  3. If the workspace has at least one starter project, but none are selected,
+//     the name of the first starter project will be used as the project source path.


I think this may break more than it fixes. If there are starterProjects but none selected, then $PROJECT_SOURCE will point to a nonexistent directory since project-clone won't clone anything.

It might be better to just take 1, 2, and 4 from this list. From my understanding, what Che will do is automatically "select" the first starterProject anyways, so this won't break their use case either.

I think this may break more than it fixes. If there are starterProjects but none selected, then $PROJECT_SOURCE will point to a nonexistent directory since project-clone won't clone anything.

Good catch, I hadn't considered that. Keeping points 1,2 and 4 as suggested.

amisevsk · 2023-10-17T16:35:30Z

pkg/library/container/mountSources.go

+	projects := projectslib.GetAllProjects(workspace)
 	if len(projects) > 0 {
+		selectedStarterProject, err := projectslib.GetStarterProject(workspace)
+		if err != nil {
+			return "", err
+		}
+		// Only use the selected starterProject if we have no regular projects
+		if selectedStarterProject != nil && len(workspace.Projects) == 0 {
+			return selectedStarterProject.Name, nil
+		}
+
 		firstProject := projects[0]
 		if firstProject.ClonePath != "" {
-			projectPath = firstProject.ClonePath
-		} else {
-			projectPath = firstProject.Name
+			return firstProject.ClonePath, nil
 		}
+		return firstProject.Name, nil
 	}
-	return projectPath
+	return "", nil


This still looks strange to me -- I think there's some duplication of work here since both projectslib.GetAllProjects() and projectslib.GetStarterProject() check for the selected starter project, so we're searching through workspace.StarterProjects twice. We're also aliasing projects to refer to "all projects", which is confusing given that there's a workspace.Projects

Would it make more sense to do something like

// If there are any projects, return the first one's clone path if len(workspace.Projects) > 0 { return projectslib.GetClonePath(workspace.Projects[0]) } // No projects, check for starter project starterProject, err := projectslib.GetStarterProject(workspace) if err != nil { return "", err } else if starterProject == nil { return "", nil } return starterProject.Name

This approach also won't throw an error here (but maybe will elsewhere) if there's a regular project but the selected starter project is misconfigured.

I agree this is a better approach and is a lot more obvious as to what's going on (especially without requiring the list of projects to be sorted with regular projects first, then starter projects second). This also eliminates the need for projectslib.GetAllProjects(), which felt a bit awkward of a function anyhow.

amisevsk · 2023-10-17T16:39:44Z

project-clone/main.go

@@ -38,7 +38,6 @@ const (
 	tmpLogFilePath = "/tmp/" + logFileName
 )

-// TODO: Handle sparse checkout
 func main() {


You might consider also moving GetClonePath to the projects library, since we're doing it both in container and project-clone

This comment is here because it's the only file in project-clone in this PR, this has nothing to do with removing the TODO (thanks!) :D

I ended up moving GetClonePath since it made sense anyhow :P

amisevsk · 2023-10-17T16:41:08Z

pkg/library/container/mountSources.go

+		if selectedStarterProject != nil && len(workspace.Projects) == 0 {
+			return selectedStarterProject.Name, nil
+		}
+
 		firstProject := projects[0]


This relies on GetAllProjects returning the projects with starterProjects being in the list after regular projects, which should either be documented as a requirement on the function (and maybe have a test case to verify) or be handled differently.

IMO it's easier to just check workspace.Projects first, and if it's empty check workspace.StarterProjects.

AObuchow · 2023-10-17T19:32:09Z

@amisevsk Thank you for the great feedback, this PR is looking a lot better now. I also updated the PR description & test cases, as well as the requirements on the original issue to specify that the starterProject must be selected in order to be used for $PROJECT_SOURCE

amisevsk

LGTM 👍

openshift-ci · 2023-10-18T21:16:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, AObuchow

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~~ [amisevsk]

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

openshift-ci · 2023-10-19T14:39:24Z

New changes are detected. LGTM label has been removed.

Signed-off-by: Andrew Obuchowicz <[email protected]>

Fix devfile#1189 Signed-off-by: Andrew Obuchowicz <[email protected]>

Support for sparse checkout was added in ece5b67 Signed-off-by: Andrew Obuchowicz <[email protected]>

AObuchow requested review from amisevsk and l0rd as code owners October 13, 2023 16:38

amisevsk reviewed Oct 13, 2023

View reviewed changes

AObuchow force-pushed the starter-projects-source-env branch 2 times, most recently from 0f3b6c6 to 49ee964 Compare October 16, 2023 19:42

amisevsk reviewed Oct 17, 2023

View reviewed changes

AObuchow force-pushed the starter-projects-source-env branch from 5d50c82 to e1a656d Compare October 17, 2023 19:23

amisevsk approved these changes Oct 18, 2023

View reviewed changes

openshift-ci bot assigned amisevsk Oct 18, 2023

openshift-ci bot added the lgtm label Oct 18, 2023

openshift-ci bot added the approved label Oct 18, 2023

AObuchow force-pushed the starter-projects-source-env branch from e1a656d to d3a3639 Compare October 19, 2023 14:39

openshift-ci bot removed the lgtm label Oct 19, 2023

AObuchow added 3 commits October 19, 2023 10:40

Move GetClonePath() to /pkg/library/projects/

48d89ea

Signed-off-by: Andrew Obuchowicz <[email protected]>

Fix: include selected starter project for $PROJECT_SOURCE path selection

c72a5d7

Fix devfile#1189 Signed-off-by: Andrew Obuchowicz <[email protected]>

Remove completed TODO comment for projectClone

f13b5fe

Support for sparse checkout was added in ece5b67 Signed-off-by: Andrew Obuchowicz <[email protected]>

AObuchow force-pushed the starter-projects-source-env branch from d3a3639 to f13b5fe Compare October 19, 2023 14:40

AObuchow merged commit c654384 into devfile:main Oct 19, 2023
5 of 6 checks passed

AObuchow deleted the starter-projects-source-env branch October 19, 2023 15:46

AObuchow mentioned this pull request Oct 19, 2023

When a devfile starterProject is used the $PROJECT_SOURCE variable is misconfigured eclipse-che/che#22601

Closed

AObuchow mentioned this pull request Jul 31, 2024

$PROJECT_SOURCE incorrectly set when starterProjects are used in devfile located in git repository eclipse-che/che#23065

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: include starter projects for $PROJECT_SOURCE path selection #1190

Fix: include starter projects for $PROJECT_SOURCE path selection #1190

AObuchow commented Oct 13, 2023 •

edited

Loading

codecov bot commented Oct 13, 2023 •

edited

Loading

amisevsk Oct 13, 2023

AObuchow Oct 13, 2023

amisevsk Oct 13, 2023 •

edited

Loading

AObuchow Oct 16, 2023 •

edited

Loading

l0rd Oct 16, 2023

AObuchow commented Oct 16, 2023

amisevsk left a comment

amisevsk Oct 17, 2023

amisevsk Oct 17, 2023

AObuchow Oct 17, 2023

amisevsk Oct 17, 2023

AObuchow Oct 17, 2023 •

edited

Loading

amisevsk Oct 17, 2023

amisevsk Oct 17, 2023

AObuchow Oct 17, 2023

amisevsk Oct 17, 2023

AObuchow commented Oct 17, 2023

amisevsk left a comment

openshift-ci bot commented Oct 18, 2023

openshift-ci bot commented Oct 19, 2023

		// 3. If the workspace has at least one starter project, but none are selected,
		// the name of the first starter project will be used as the project source path.

Fix: include starter projects for $PROJECT_SOURCE path selection #1190

Fix: include starter projects for $PROJECT_SOURCE path selection #1190

Conversation

AObuchow commented Oct 13, 2023 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

devworkspace with only starter projects

devworkspace with starter projects and regular projects

devworkspace with only starter projects, with a starter project being selected

devworkspace with regular and starter projects, with a selected starter project

PR Checklist

codecov bot commented Oct 13, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amisevsk Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

AObuchow Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AObuchow commented Oct 16, 2023

amisevsk left a 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

AObuchow Oct 17, 2023 • 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

AObuchow commented Oct 17, 2023

amisevsk left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 18, 2023

openshift-ci bot commented Oct 19, 2023

AObuchow commented Oct 13, 2023 •

edited

Loading

codecov bot commented Oct 13, 2023 •

edited

Loading

amisevsk Oct 13, 2023 •

edited

Loading

AObuchow Oct 16, 2023 •

edited

Loading

AObuchow Oct 17, 2023 •

edited

Loading