Remove GitHub projects #439

amisevsk · 2021-04-26T17:36:07Z

What does this PR do?

Remove projects with type GitHub, since this duplicates Git-type projects.

I'm not sure this PR should be merged now, as GitHub projects were deprecated on Feb 19 but I think it makes sense to include this in the 2.1 milestone as the change will be required by the time v1alpha2 is set (since it's a k8s-incompatible change -- doing it later would require going to v1alpha3). As far as I know, no devfiles/DevWorkspaces use GitHub projects so no use cases should be broken.

What issues does this PR fix or reference?

Fixes #340

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

Fixed conversion tests to ignore GitHub projects.

sleshchenko

Seems tests needs to be adapted

schema_test.go:222: OVERALL FAIL : 34 of 291 tests failed.

but everything else LGTM

openshift-ci-robot · 2021-04-27T07:56:57Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

OWNERS

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

yangcao77 · 2021-04-27T18:15:33Z

@amisevsk I have created a PR in your branch to fix the test failures, can you help review and merge if no issues with it: amisevsk#1

openshift-ci-robot · 2021-04-27T18:42:47Z

New changes are detected. LGTM label has been removed.

amisevsk · 2021-04-27T18:48:22Z

@yangcao77 ahh sorry, I missed this comment until right after I pushed 18525d4 and 6726b05!

I think I basically did the same thing as you :)

yangcao77 · 2021-04-27T20:53:15Z

pkg/apis/workspaces/v1alpha1/projects_conversion.go

@@ -19,8 +19,6 @@ func convertProjectTo_v1alpha2(src *Project, dest *v1alpha2.Project) error {
 	switch {
 	case src.Git != nil:
 		sparseCheckoutDir = src.Git.SparseCheckoutDir
-	case src.Github != nil:
-		sparseCheckoutDir = src.Github.SparseCheckoutDir


since Github is duplicate of Git. Should we convert v1.Github to v2.Git, instead of ignoring it?

Hm, this might be a good idea. My first impulse is to not do it since it would change the file on round trip (i.e. v1alpha1 (GitHub) -> v1alpha2 (Git) -> v1alpha1 (Git)), but my approach of ignoring the problem just changes the file in a different way.

On the other hand, if there's not a whole lot of value in it, it's a lot simpler to not bother. The way conversion is implemented (round trip through json) would mean we'd have to do something fancy to make it work.

emm. When you say "round trip conversion", did you mean how the conversion test is implemented?
I agree more extra work needs to be done if we want to test github->git conversion. however, the ConvertTo (

api/pkg/apis/workspaces/v1alpha1/devworkspace_conversion.go

Lines 11 to 14 in a313872

func (src *DevWorkspace) ConvertTo(destRaw conversion.Hub) error {

dest := destRaw.(*v1alpha2.DevWorkspace)

return convertDevWorkspaceTo_v1alpha2(src, dest)

}

) function is exposed, and can be called by consumer. so we probably should not simply ignore it.
what if we still convert v1alpha1 (GitHub) -> v1alpha2 (Git) in the actual conversion function, and just ignore Github case in the conversion test?

The way conversion is supposed to be implemented (from a k8s perspective) is that converting from version X to the hub version Y (v1alpha2 in our case) and back again should result in unmodified data. This is because internally, when you apply a v1alpha1 DevWorkspace to your cluster, kube will convert it to v1alpha2 and store that in etcd. This v1alpha2 spec is converted back to v1alpha1 when you request it from the cluster -- this is what the conversion tests are testing.

The way this is often done for removed/added fields is to serialize removed fields into a json annotation so that when you unconvert you get the same data, but IMO this is a very messy approach for a feature nobody is using (i.e. a bunch of work for no gain). We've done a similar thing for e.g. the vscode command types.

The additional work that would be required to implement GitHub -> Git conversion is due to the approach we take to conversion, where we take version X, serialize it to json, and then deserialize it to version Y (e.g. here). This was done since the main breaking change between v1alpha1 and v1alpha2 is moving component name, command id, etc. one level higher in the spec (i.e. .spec.components[].name vs .spec.components[].container.name, which makes the types not assignable and makes conversion-gen code nearly useless. By going through json, we can easily assign all fields shared between v1alpha1 and v1alpha2, but this doesn't work if the path is different (a GitHub project -- e.g. .projects[].github.remotes -- will deserialize to nil if you pass in a Git project).

I can definitely look into it, but I'm hesitant to introduce complexity in the conversion code to cover a case that (I think) will never actually be called in production.

I'm thinking if adding an attribute when convert Github -> Git would help in this situation?

I finally got around to this: 45b3cff

Please take a look.

The basic gist of the approach is

v1alpha1 | intermediate (v1alpha1) | v1alpha2 -----------------------------|--------------------------------|-------------------------------- projects: | projects: | projects: - name: project | - name: project | - name: project github: | github: | remotes: | remotes: | origin: my-url.com | origin: my-url.com | | git: | git: | remotes: | remotes: | origin: my-url.com | origin: my-url.com

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

pkg/apis/workspaces/v1alpha1/projects_conversion.go

yangcao77

changes looks good to me

openshift-ci · 2021-05-12T14:59:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, sleshchenko, yangcao77

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

Needs approval from an approver in each of these files:

OWNERS

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

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

When converting a devfile in v1alpha1 that contains projects with type Github, convert those projects to Git-type projects in v1alpha2 and apply attribute 'conversion.api.devfile.io/converted-from=GitHub' to them. When converting from v1alpha2, convert Git-type projects with the above attribute back to Github-type projects in v1alpha1. Reenable testing conversion for Github-type projects in v1alpha1 now that it is supported. Signed-off-by: Angel Misevski <[email protected]>

openshift-ci · 2021-05-13T01:51:24Z

New changes are detected. LGTM label has been removed.

amisevsk requested review from kadel, davidfestal and sleshchenko April 26, 2021 17:36

sleshchenko approved these changes Apr 27, 2021

View reviewed changes

openshift-ci-robot assigned sleshchenko Apr 27, 2021

openshift-ci-robot added the lgtm label Apr 27, 2021

yangcao77 mentioned this pull request Apr 27, 2021

remove github support from project #442

Closed

openshift-ci-robot removed the lgtm label Apr 27, 2021

amisevsk requested a review from yangcao77 April 27, 2021 19:54

yangcao77 reviewed Apr 27, 2021

View reviewed changes

openshift-ci bot added the needs-rebase label May 9, 2021

amisevsk added 2 commits May 10, 2021 19:23

Remove GitHub-type projects from v1alpha2 spec

5441a12

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

Regenerate CRDs and schemas after removing GitHub-type projects

a6f0a78

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

amisevsk force-pushed the remove-github-projects branch from 6726b05 to 9d476bc Compare May 11, 2021 02:12

openshift-ci bot removed the needs-rebase label May 11, 2021

amisevsk requested a review from yangcao77 May 11, 2021 18:48

yangcao77 requested changes May 11, 2021

View reviewed changes

pkg/apis/workspaces/v1alpha1/projects_conversion.go Outdated Show resolved Hide resolved

pkg/apis/workspaces/v1alpha1/projects_conversion.go Outdated Show resolved Hide resolved

openshift-ci bot assigned yangcao77 May 11, 2021

yangcao77 approved these changes May 12, 2021

View reviewed changes

openshift-ci bot added the lgtm label May 12, 2021

amisevsk added 2 commits May 12, 2021 21:50

Fix schema and validation tests to not include github projects

a5a1cda

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

amisevsk force-pushed the remove-github-projects branch from 6126fb6 to d2c5a8f Compare May 13, 2021 01:51

openshift-ci bot removed the lgtm label May 13, 2021

amisevsk merged commit 3ac0baa into devfile:main May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove GitHub projects #439

Remove GitHub projects #439

amisevsk commented Apr 26, 2021 •

edited by yangcao77

Loading

sleshchenko left a comment

openshift-ci-robot commented Apr 27, 2021

yangcao77 commented Apr 27, 2021 •

edited

Loading

openshift-ci-robot commented Apr 27, 2021

amisevsk commented Apr 27, 2021 •

edited

Loading

yangcao77 Apr 27, 2021

amisevsk Apr 27, 2021

yangcao77 Apr 27, 2021 •

edited

Loading

amisevsk Apr 28, 2021 •

edited

Loading

yangcao77 Apr 28, 2021

amisevsk May 11, 2021

amisevsk May 11, 2021

yangcao77 left a comment

openshift-ci bot commented May 12, 2021

openshift-ci bot commented May 13, 2021

	func (src *DevWorkspace) ConvertTo(destRaw conversion.Hub) error {
	dest := destRaw.(*v1alpha2.DevWorkspace)
	return convertDevWorkspaceTo_v1alpha2(src, dest)
	}

Remove GitHub projects #439

Remove GitHub projects #439

Conversation

amisevsk commented Apr 26, 2021 • edited by yangcao77 Loading

What does this PR do?

What issues does this PR fix or reference?

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

sleshchenko left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Apr 27, 2021

yangcao77 commented Apr 27, 2021 • edited Loading

openshift-ci-robot commented Apr 27, 2021

amisevsk commented Apr 27, 2021 • edited Loading

yangcao77 Apr 27, 2021

Choose a reason for hiding this comment

amisevsk Apr 27, 2021

Choose a reason for hiding this comment

yangcao77 Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

amisevsk Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

yangcao77 Apr 28, 2021

Choose a reason for hiding this comment

amisevsk May 11, 2021

Choose a reason for hiding this comment

amisevsk May 11, 2021

Choose a reason for hiding this comment

yangcao77 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented May 12, 2021

openshift-ci bot commented May 13, 2021

amisevsk commented Apr 26, 2021 •

edited by yangcao77

Loading

yangcao77 commented Apr 27, 2021 •

edited

Loading

amisevsk commented Apr 27, 2021 •

edited

Loading

yangcao77 Apr 27, 2021 •

edited

Loading

amisevsk Apr 28, 2021 •

edited

Loading