Closed amisevsk closed 10 months ago
@amisevsk looks awesome to me so far 🥳 Will aim to do a full review on Monday.
Attention: 16 lines
in your changes are missing coverage. Please review.
Comparison is base (
c654384
) 52.96% compared to head (7966462
) 52.81%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@svor @RomanNikitenko If you have some time, please also take a look at this PR and leave review comments (I can't explicitly request review from you). This is somewhat related to the devworkspace generator and the "Restart from local Devfile" functionality in Code (e.g. this PR).
This PR partly reimplements the logic/goal of the generator's generateDevfileContext
, with some different assumptions (basically, we assume someone else can handle the editor, etc.). My reasoning here is that "Update DevWorkspace using an in-repo devfile" amounts to:
.spec.template
If you would like me to contribute something similar to the generator, let me know :)
@amisevsk
as far as I understand - we need similar changes at least for the generator - correct?
and
probably some changes on the che-code
side - to handle the use case with merging projects
as far as I understand - we need similar changes at least for the generator - correct?
I don't think the generator or che-code needs to be updated, and there's not really an issue on that side of things:
On the other hand, the DevWorkspace/devfile/Che APIs have reached a level of complexity that it's probably worth our time to discuss and make sure our interpretations of how things should fit together are in line -- I don't want to do something in DWO that is unexpected from the perspective of the devworkspace-generator library.
As for projects, the implementation here is basically just a temporary workaround until dependentProjects are added to the devfile API. After dependentProjects is introduced, handling this is a fair bit simpler :)
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: amisevsk, AObuchow, svor
The full list of commands accepted by this bot can be found here.
The pull request process is described here
I'm curious where this flow could be useful/applicable in case of Che/DS? Maybe in tests that don't use UI and were we need to have a final DevWorkspace, but we still need to create and store short version of DevWorkspace with bootstrap-devworkspace attribute.
I don't have anything specific in mind, it was just brought up that this change may be of interest to the generator (and prompted me to look into the generator as well to see how it works). I agree that the generator is doing something different (and needs to generate e.g. devworkspacetemplates, etc.).
One area we could potentially improve is https://github.com/eclipse/che/issues/22614 -- currently the generator will set environment variables on all containers when converting the devfile, and I'm not sure that's still necessary with the move to container contributions (which merge environment variables from the contribution). As far as I know, those Che-specific environment variables are needed only by the editor, so we could probably get away with not setting them in any devfile containers. This would also make bootstrapping "safer" since the result of bootstrapping would be more similar to the result of the generator.
What does this PR do?
Adds DevWorkspace attribute
controller.devfile.io/bootstrap-devworkspace
. If set totrue
, this attribute will configure project clone to do the following:What issues does this PR fix or reference?
Closes #1192
Is it tested? How?
Basic testing of this PR is to create a DevWorkspace that uses the attribute and imports a project with a devfile.yaml at its root:
After starting, the workspace should update so that the
.spec.template
of the workspace matches the devfile of the imported project, with the merge process above (attributes, projects are merged instead of overwritten)PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-path
to trigger)v8-devworkspace-operator-e2e
: DevWorkspace e2e testv8-che-happy-path
: Happy path for verification integration with Che