Skip to content

Re-add the v1alpha1 API and implement conversion code #213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Nov 24, 2020

Conversation

amisevsk
Copy link
Contributor

@amisevsk amisevsk commented Nov 9, 2020

This PR should be reviewed commit-by-commit

What does this PR do?

  • Re-add the go files defining the v1alpha1 API (this tag) to the current repo
  • Implement conversion between v1alpha2 and v1alpha1 as much as possible (see below) according to the standard k8s spec (doc)
    • v1alpha2 is defined as the hub and the storage version.
  • Add tests for conversion code.
  • Regenerate files where appropriate to include v1alpha1.

Note that the general k8s requirement is that all CRD versions are safely round-trippable, meaning we can freely convert between them with no loss of information. This is not possible for DevWorkspaces due to some removals in v1alpha2 (e.g. removing custom commands and components from parents). I've added a few fields to the v1alpha1 spec where appropriate, so that we're mostly able to safely convert v1alpha2 -> v1alpha1 -> v1alpha2 without issue (the exception is sparseCheckoutDirs on Projects)

The general approach in this PR is to rely on yaml marshalling and unmarshalling. Otherwise a huge amount of code is required. Attempting to use conversion-gen results in an auto-generated file that's 2500 lines long, and still doesn't implement the bulk of the conversion.

Is your PR tested? Consider putting some instruction how to test your changes

This PR includes tests where the general structure is to fuzz a v1alpha1 struct, convert it v1alpha1 -> v1alpha2 -> v1alpha1 and check that it is unchanged.

I've also included a simple benchmark for conversion. Using a mostly-filled devworkspace, conversion takes around 8ms 5ms on my machine.

To test the code, use

go test ./pkg/apis/workspaces/v1alpha1 --count=1 -test.v

To run the benchmark,

go test ./pkg/apis/workspaces/v1alpha1 -bench=. -run=none -test.v

Additional Info

Summary of additions to v1alpha1 in this PR:

With the additions above, the differences between v1alpha1 and v1alpha2 are

  • Keys (ID/Name) are generally moved a level up in the yaml structure for Components, Commands, etc.
  • Custom Components are removed from Plugin components and Parent
  • Custom Commands are removed from Plugin components and Parent.
  • Structure of Projects is significantly changed:
    • Custom project type is removed from Parent
    • sparseCheckoutDir is moved a level up and converted to a list. Since with v1alpha2, we can have multiple
    • For starter projects, clonePath and sparseCheckoutDir are removed; subDir is added. For compatibility, I use subDir to hold the value of sparseCheckoutDir.

A summary of differences can be found in this diff

@maysunfaisal
Copy link
Member

@amisevsk curious as to why we're adding this and the conversion back when we didnt release during v1alpha1 :)

@amisevsk
Copy link
Contributor Author

@maysunfaisal Because the Web Terminal Operator is released (you can grab it on crc now) and uses the v1alpha1 API.

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job. Nothing meaningful to add )
It was pretty craftily to use yaml marshaling to reduce our code for converting, as we see it's fast enough to proceed with.

@amisevsk
Copy link
Contributor Author

Rebased on master to resolve a conflict with #211

@amisevsk amisevsk force-pushed the add-conversion-v1alpha1 branch from 6946ab9 to 88e8f94 Compare November 11, 2020 02:42
@amisevsk
Copy link
Contributor Author

Fixed a rare test flake when gofuzz would generate a map[string]string with the yaml merge key (<<), breaking yaml serialization/deserialization.

amisevsk added a commit to amisevsk/devworkspace-operator that referenced this pull request Nov 17, 2020
In order to use v1alpha2 with conversion from v1alpha1, we need to use
the current (open) PR in devfile/api that combines the API versions and
implements conversion. This commit updates go.mod and related files to
point to the head of devfile/api#213

Signed-off-by: Angel Misevski <[email protected]>
@amisevsk
Copy link
Contributor Author

As suggested by @davidfestal, I've updated conversion to use json as an intermediate representation, since the yaml package we use converts between yaml and json as an intermediate step.

@amisevsk amisevsk force-pushed the add-conversion-v1alpha1 branch from 79cdf01 to 050e283 Compare November 20, 2020 19:34
Copy link
Contributor

@JPinkney JPinkney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@davidfestal davidfestal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, but you will have to rebase the PR on master since I merged PR #214, so that endpoint attributes should be updated from string[string]string to Attributes.

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, davidfestal, JPinkney, sleshchenko
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

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

Needs approval from an approver in each of these files:

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

Copied from commit 9e2280a

Signed-off-by: Angel Misevski <[email protected]>
Projects bootstrapped by the current version of kubebuilder depend on a
.AddToScheme method being available.

Signed-off-by: Angel Misevski <[email protected]>
@amisevsk amisevsk force-pushed the add-conversion-v1alpha1 branch from 050e283 to 00a3487 Compare November 23, 2020 20:57
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

Signed-off-by: Angel Misevski <[email protected]>
Add functions to correctly generate devworkspaces (respecting oneOf
requirements)

Signed-off-by: Angel Misevski <[email protected]>
When converting between v1alpha1 and v1alpha2, use the json
serialization instead of the yaml serialization, since we only need to
serialize and deserialize. The yaml package converts to/from json as an
intermediate step.

Signed-off-by: Angel Misevski <[email protected]>
* Add conversion functions for going from map[string]string to
attributes.Attributes
* Implement conversion of command attributes for Commands
* Adapt tests to handle new incompatibility (parent commands cannot have
attributes)

Signed-off-by: Angel Misevski <[email protected]>
@amisevsk amisevsk force-pushed the add-conversion-v1alpha1 branch from 00a3487 to 7ef3a7b Compare November 24, 2020 04:38
@amisevsk
Copy link
Contributor Author

Rebased on master to include changes from #214

I had to write some additional conversion code, since #214 moves attributes on commands up a level (from e.g. command.Exec.Attributes to command.Attributes. It also removes attributes from Parent commands, which is not reflected in the test cases.

In the general case, no changes were needed for converting attributes in other cases.

@amisevsk
Copy link
Contributor Author

Merging this PR as it's been two weeks; if any further issues are brought forward I'll fix them separately.

@amisevsk amisevsk merged commit 1103496 into devfile:master Nov 24, 2020
@sleshchenko
Copy link
Member

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants