Add support for dependentProjects in devfiles #1205

amisevsk · 2023-11-16T22:17:44Z

What does this PR do?

Adds support for dependentProjects as part of a DevWorkspace. As part of this, this PR also adds an additional validation step, to ensure no two projects:

Have the same name (possible now since dependentProjects are a separate field)
Have the same clone path (and would thus collide).

What issues does this PR fix or reference?

Closes #1204

Is it tested? How?

Note: testing this PR requires running make install from the PR changes to ensure the updated DevWorkspace CRDs are installed.

Can be tested via the following devfile:

kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: dependentprojects-test
spec:
  started: true
  template:
    attributes:
      controller.devfile.io/use-starter-project: test-starter-project-2
    projects:
      - name: test-regular-project-1
        git:
          remotes:
            origin: "https://github.com/che-samples/web-nodejs-sample.git"
      - name: test-regular-project-2
        git:
          remotes:
            origin: "https://github.com/che-samples/web-nodejs-sample.git"
    dependentProjects:
      - name: test-dependent-project-1
        git:
          remotes:
            origin: "https://github.com/che-samples/web-nodejs-sample.git"
      - name: test-dependent-project-2
        git:
          remotes:
            origin: "https://github.com/che-samples/web-nodejs-sample.git"
    starterProjects: 
      - name: test-starter-project-1
        git:
          remotes:
            origin: "https://github.com/che-samples/web-nodejs-sample.git"
      - name: test-starter-project-2
        git:
          remotes:
            origin: "https://github.com/che-samples/web-nodejs-sample.git"
    components:
      - name: dev
        container:
          image: quay.io/devfile/universal-developer-image:latest
          memoryLimit: 512Mi
          memoryRequest: 256Mi
          cpuRequest: 1000m
  contributions:
    - name: che-code
      uri: https://eclipse-che.github.io/che-plugin-registry/main/v3/plugins/che-incubator/che-code/latest/devfile.yaml
      components:
        - name: che-code-runtime-description
          container:
            env:
              - name: CODE_HOST
                value: 0.0.0.0

This workspace should clone five projects:

projects $ ls -l /projects
total 20
drwxr-sr-x. 6 user user 4096 Nov 16 22:03 test-dependent-project-1
drwxr-sr-x. 6 user user 4096 Nov 16 22:03 test-dependent-project-2
drwxr-sr-x. 6 user user 4096 Nov 16 22:03 test-regular-project-1
drwxr-sr-x. 6 user user 4096 Nov 16 22:03 test-regular-project-2
drwxr-sr-x. 6 user user 4096 Nov 16 22:03 test-starter-project-2

To verify the validation steps, project names/clonePaths can be changed.

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

Signed-off-by: Angel Misevski <[email protected]>

amisevsk · 2023-11-17T21:00:09Z

/retest

codecov · 2023-11-17T21:47:50Z

Codecov Report

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

Comparison is base (9fcb073) 52.89% compared to head (8662525) 52.75%.
Report is 7 commits behind head on main.

Files	Patch %	Lines
webhook/workspace/handler/validate.go	0.00%	6 Missing ⚠️
controllers/workspace/devworkspace_controller.go	0.00%	2 Missing and 1 partial ⚠️
pkg/library/container/mountSources.go	0.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1205      +/-   ##
==========================================
- Coverage   52.89%   52.75%   -0.14%     
==========================================
  Files          84       84              
  Lines        7595     7616      +21     
==========================================
+ Hits         4017     4018       +1     
- Misses       3291     3309      +18     
- Partials      287      289       +2

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

AObuchow

Some minor comments below.

Also, the devfile API does not specify if it's valid to have a dependentProject but no regular projects.

I think it's outside the scope of DWO to do this validation, but it might make sense to make a bug about this for the devfile API's validation - not that this is a big deal IMO; I don't anticipate users to make devfiles with only dependentProjects and no regular projects.

AObuchow · 2023-11-21T21:50:20Z

pkg/library/projects/clone.go

+
+	for idx, project := range workspace.DependentProjects {
+		projectNames[project.Name] = append(projectNames[project.Name], "dependentProjects")
+		clonePath := GetClonePath(&workspace.Projects[idx])


This should be clonePath := GetClonePath(&project), and optionally you could get rid of idx for both loops:

for _, project := range workspace.Projects { projectNames[project.Name] = append(projectNames[project.Name], "projects") clonePath := GetClonePath(&project) clonePaths[clonePath] = append(clonePaths[clonePath], fmt.Sprintf("project %s", project.Name)) } for _, project := range workspace.DependentProjects { projectNames[project.Name] = append(projectNames[project.Name], "dependentProjects") clonePath := GetClonePath(&project) clonePaths[clonePath] = append(clonePaths[clonePath], fmt.Sprintf("dependentProject %s", project.Name)) }

The use of idx here is because we're taking the reference of the project. Some linters will complain about implicit memory aliasing here (since &project is the same address every loop iteration). See e.g. the discussion here.

Doesn't impact us here since GetClonePath is just reading fields, but I'd still rather avoid a future false positive. This is something that's going to be fixed in Go 1.22

Thank you for the explaining the context behind that pattern & glad to see it'll be fixed in the future :)

I should have been a bit more explicit, though: there's a subtle typo/bug. We're iterating over workspace.DependentProjects but accessing &workspace.Projects[idx] instead of &workspace.DependentProjects[idx]. This causes validation to fail.

Ahhh makes sense; thank you!

No problem :)

I saw you force pushed some more commits, but this line still seems to use &workspace.Projects[idx] instead of &workspace.DependentProjects[idx]

pkg/library/projects/clone.go

amisevsk · 2023-11-28T19:58:12Z

Also, the devfile API does not specify if it's valid to have a dependentProject but no regular projects.

Given the purpose of dependentProjects, it's safe to assume such a devfile is valid. Most in-repo devfiles are assumed to not have a project included, since it's inferred from the containing repository.

Add verification step when starting workspace to make sure that no two projects (/dependentProjects/starterProjects) share the same name nor have the same clone path. Signed-off-by: Angel Misevski <[email protected]>

AObuchow

LGTM, validation works as expected as well in my testing 👍

openshift-ci · 2023-11-30T21:33:13Z

[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

Add support for dependentProjects in DevWorkspaces

53eef37

Signed-off-by: Angel Misevski <[email protected]>

amisevsk requested review from AObuchow and l0rd as code owners November 16, 2023 22:17

openshift-ci bot added the approved label Nov 16, 2023

This was referenced Nov 16, 2023

fix: launcher should update projects in code-workspace file at workspace startup che-incubator/che-code#302

Merged

Add dependent projects supports in VS Code launcher eclipse-che/che#22629

Closed

AObuchow reviewed Nov 21, 2023

View reviewed changes

amisevsk force-pushed the dependentProjects-support branch 2 times, most recently from 1eef996 to 5299844 Compare November 28, 2023 20:17

Validate that projects and dependentProjects do not collide

8662525

Add verification step when starting workspace to make sure that no two projects (/dependentProjects/starterProjects) share the same name nor have the same clone path. Signed-off-by: Angel Misevski <[email protected]>

amisevsk force-pushed the dependentProjects-support branch from 5299844 to 8662525 Compare November 29, 2023 20:46

AObuchow approved these changes Nov 30, 2023

View reviewed changes

openshift-ci bot assigned AObuchow Nov 30, 2023

openshift-ci bot added the lgtm label Nov 30, 2023

amisevsk merged commit 472fc3a into devfile:main Dec 1, 2023
4 of 6 checks passed

amisevsk deleted the dependentProjects-support branch December 1, 2023 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for dependentProjects in devfiles #1205

Add support for dependentProjects in devfiles #1205

amisevsk commented Nov 16, 2023

amisevsk commented Nov 17, 2023

codecov bot commented Nov 17, 2023 •

edited

Loading

AObuchow left a comment •

edited

Loading

AObuchow Nov 21, 2023

amisevsk Nov 28, 2023 •

edited

Loading

AObuchow Nov 29, 2023

amisevsk Nov 29, 2023

AObuchow Nov 29, 2023

amisevsk commented Nov 28, 2023 •

edited

Loading

AObuchow left a comment

openshift-ci bot commented Nov 30, 2023

Add support for dependentProjects in devfiles #1205

Add support for dependentProjects in devfiles #1205

Conversation

amisevsk commented Nov 16, 2023

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

PR Checklist

amisevsk commented Nov 17, 2023

codecov bot commented Nov 17, 2023 • edited Loading

Codecov Report

AObuchow left a comment • edited Loading

Choose a reason for hiding this comment

AObuchow Nov 21, 2023

Choose a reason for hiding this comment

amisevsk Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

AObuchow Nov 29, 2023

Choose a reason for hiding this comment

amisevsk Nov 29, 2023

Choose a reason for hiding this comment

AObuchow Nov 29, 2023

Choose a reason for hiding this comment

amisevsk commented Nov 28, 2023 • edited Loading

AObuchow left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 30, 2023

codecov bot commented Nov 17, 2023 •

edited

Loading

AObuchow left a comment •

edited

Loading

amisevsk Nov 28, 2023 •

edited

Loading

amisevsk commented Nov 28, 2023 •

edited

Loading