Manage merge keys of top-level lists in a more consistent way #122

kadel · 2020-08-20T13:46:24Z

What does this PR do?

moves component and command merge keys (name and id) to top-level
This change requires updates to utility functions for handling unions (union.go). I was not able to found if this is being used somewhere. So I tried to write unit tests for it. I'm not sure if those functions actually ever worked correctly :-(

TODO:

update union.go and tests

What issues does this PR fix or reference?

fixes #102

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

Docs PR

amisevsk · 2020-08-20T20:47:33Z

@kadel I believe the union/etc. functions will be updated in #98, if you feel like reviewing a 10k PR ;). That PR also adds a good amount of tests.

amisevsk

Generally LGTM, just a couple comments.

It looks like we'll need to figure out how to best merge this vs #98, as they'll likely conflict at least somewhat. Probably it makes sense to merge this one and figure out #98 afterwards, since David is on PTO anyways.

pkg/apis/workspaces/v1alpha1/union_test.go

pkg/apis/workspaces/v1alpha1/components.go

kadel · 2020-08-21T14:30:01Z

@amisevsk with regards to #111 Should I move all changes in this PR to v1alpha2?

amisevsk · 2020-08-21T20:47:53Z

@kadel Ah yeah I can't believe I forgot that, given that I opened the blocker issue. Yes, these changes should be applied to a v1alpha2 version -- sorry for the hassle.

kadel · 2020-08-24T08:24:11Z

@kadel Ah yeah I can't believe I forgot that, given that I opened the blocker issue. Yes, these changes should be applied to a v1alpha2 version -- sorry for the hassle.

No problem. I realized that after I opened this PR myself :-/ . Will update it today.

davidfestal · 2020-08-24T08:27:06Z

@kadel @amisevsk I merged #98 first, precisely because of the incompatibility this PR introduces in the v1alpha1 K8S API. So the merged overriding implementation supports v1alpha1, and could be then updated to also support v1alpha2 I assume.

davidfestal · 2020-08-24T17:50:40Z

@kadel Ah yeah I can't believe I forgot that, given that I opened the blocker issue. Yes, these changes should be applied to a v1alpha2 version -- sorry for the hassle.

No problem. I realized that after I opened this PR myself :-/ . Will update it today.

So after discussing the situation again, the plan would be:

Create a branch (or tag) on master with the existing v1alpha1 API, and let the workspace controller depend on this commit.
Create and merge a distinct PR that would globally rename v1alpha1 folder and all v1alpha1mentions to v1alpha2 on the master branch
Update this PR to merge it with the changes made in PR Implementation of parent and plugin overriding #98
Merge this branch on master.
On the workspace controller side, start working on the reintegration of the v1alpha1 + conversion webhooks, on a distinct PR branch
When this last PR is merged, the workspace controller would be able to support both v1alpha1 and v1alpha2 with the conversion webhook.

So the main point here is that this PR should wait for the steps 1 and 2 to be finished.

@amisevsk please complete or correct this if necessary.

amisevsk · 2020-08-25T03:45:59Z

@amisevsk please complete or correct this if necessary.

The above outline is basically it. There's some grey area about how integration will work down the line, but the main idea is to "fix" v1alpha1 in a tag/branch, and only support v1alpha2 in the repo for now, with the goal of implementing converstion webhooks, etc. before "releasing" v1alpha2. The reason I think we can do this is because the devworkspace controller depends on the api and crds, whereas odo only depends on the APIs.

pkg/apis/workspaces/v1alpha1/commands.go

davidfestal · 2020-08-25T08:38:51Z

pkg/apis/workspaces/v1alpha1/union.go

 	visitorValue := reflect.ValueOf(visitor)
 	unionValue := reflect.ValueOf(union)
 	oneMemberPresent := false
 	typeOfVisitor := visitorValue.Type()
 	for i := 0; i < visitorValue.NumField(); i++ {
 		unionMemberToRead := typeOfVisitor.Field(i).Name
 		unionMember := unionValue.FieldByName(unionMemberToRead)
-		if !unionMember.IsNil() {


In fact the function that should be called instead of IsNil is IsZero. I already fixed that in PR #98.

schema-transformation-rules/devworkspace/addRequiredFields.json

Signed-off-by: David Festal <[email protected]>

... since keys are now available directly in the top-level lists.

Signed-off-by: David Festal <[email protected]>

davidfestal · 2020-08-26T18:05:08Z

@kadel I removed the WIP flag since I reworked this PR to:

integrate some required refactoring of the previously-merged PR Implementation of parent and plugin overriding #98
rebase on master (which includes being now based on the new v1alpha2 K8S API)
add (as the last commit of this PR) the ability to override the starterProjects list in a child devfile.

@amisevsk You might want to review the PR again.

pkg/utils/overriding/merging.go

new schema is introduced in devfile/api#122

... to avoid referencing the top-level lists that should be merged by name. Signed-off-by: David Festal <[email protected]>

Signed-off-by: David Festal <[email protected]>

amisevsk

I have significant issues with the implementation of keys and unions as it's implemented currently. In addition, the messy commit history and big changes in this PR make reviewing that code very difficult (I can't even cleanly leave multi-line comments).

I think to unblock the core functionality of this PR, this PR should remove /pkg/utils and related api files from the repo, and a new PR should be created that re-adds those files to allow a full, straightforward review.

pkg/utils/overriding/test-fixtures/merges/duplicate-with-plugin/plugin.yaml

pkg/utils/overriding/test-fixtures/merges/events-merge/main.yaml

pkg/utils/overriding/test-fixtures/patches/add-command-and-component/patch.yaml

pkg/apis/workspaces/v1alpha2/keyed.go

pkg/utils/overriding/keys.go

pkg/utils/overriding/merging.go

cf [review comment](devfile#122 (comment)) Signed-off-by: David Festal <[email protected]>

... And use `Keyed` instead of `string` as the item type in the `TopLevelLists` type Signed-off-by: David Festal <[email protected]>

Co-authored-by: Angel Misevski <[email protected]>

Signed-off-by: David Festal <[email protected]>

according to this [review comment](devfile#122 (comment)) Signed-off-by: David Festal <[email protected]>

Signed-off-by: David Festal <[email protected]>

... according to the following [review coment](devfile#122 (comment)) Signed-off-by: David Festal <[email protected]>

Signed-off-by: David Festal <[email protected]>

Simplify checkKeys function in overriding util by separating out the merging maps step Signed-off-by: Angel Misevski <[email protected]>

amisevsk · 2020-09-01T17:35:17Z

@davidfestal I've added ef7ba59 to simplify checkKeys as we discussed (GitHub has once again chosen to break the readability of the diff by matching close parens 🙃). PTAL if you have a moment.

cc: @kadel

amisevsk

Bit strange to approve a PR after I've committed to it, but it LGTM at this point.

Fix tests that check error messages to ignore order, since Go iterates through maps in a random order. Signed-off-by: Angel Misevski <[email protected]>

kadel

LGTM 👍

new schema is introduced in devfile/api#122

kadel requested review from amisevsk, davidfestal and sleshchenko as code owners August 20, 2020 13:46

amisevsk approved these changes Aug 21, 2020

View reviewed changes

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

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

kadel force-pushed the top-level-merge-key branch from 4db2ec5 to 8cc8418 Compare August 21, 2020 14:39

kadel mentioned this pull request Aug 24, 2020

Update Devfiles and odo to work with breaking changes in devfile spec redhat-developer/odo#3822

Closed

4 tasks

davidfestal reviewed Aug 25, 2020

View reviewed changes

kadel and others added 12 commits August 26, 2020 19:14

move commands and components merge key to top-level

600f8ee

update generated schemas

11cfd4f

remove unit_test.go for now

65d38c1

update generated schemas

b726b6f

Only bring the Command ID at the top-level

defce88

Signed-off-by: David Festal <[email protected]>

Manage the key move in PluginComponentsOverride

d8c0c68

Signed-off-by: David Festal <[email protected]>

Refactor / Simplify the overriding and merging ...

fa9a641

... since keys are now available directly in the top-level lists.

Update overriding and merging test yamls

14dd37d

Signed-off-by: David Festal <[email protected]>

Update samples

c3894e0

Signed-off-by: David Festal <[email protected]>

Generate CRDs and schemas

b73fa61

Signed-off-by: David Festal <[email protected]>

Fix the mandatory name description

045d6be

Signed-off-by: David Festal <[email protected]>

Update the package name after the K8S API upgrade

e081817

Signed-off-by: David Festal <[email protected]>

davidfestal force-pushed the top-level-merge-key branch from 8cc8418 to e081817 Compare August 26, 2020 17:27

davidfestal added 3 commits August 26, 2020 19:30

Regenerate CRDs and schemas

397d6e6

Signed-off-by: David Festal <[email protected]>

Fix Go formatting

92a034b

Signed-off-by: David Festal <[email protected]>

Make starterProjects overridable by child devfile

2609a33

Signed-off-by: David Festal <[email protected]>

davidfestal changed the title ~~[WIP] Manage merge keys of top-level lists in a more consistent way~~ Manage merge keys of top-level lists in a more consistent way Aug 26, 2020

sleshchenko reviewed Aug 28, 2020

View reviewed changes

pkg/utils/overriding/merging.go Outdated Show resolved Hide resolved

kadel added a commit to kadel/registry-old that referenced this pull request Aug 28, 2020

update devfiles to match new schema

7bd7f4e

new schema is introduced in devfile/api#122

kadel mentioned this pull request Aug 28, 2020

update devfiles to match new schema odo-devfiles/registry#41

Merged

davidfestal added 3 commits August 31, 2020 15:16

Use reflection and TopLevelListContainer interface ...

03d3400

... to avoid referencing the top-level lists that should be merged by name. Signed-off-by: David Festal <[email protected]>

Format code

dde4da0

Signed-off-by: David Festal <[email protected]>

Fail-fast during overriding.

9582560

Signed-off-by: David Festal <[email protected]>

amisevsk reviewed Aug 31, 2020

View reviewed changes

davidfestal and others added 14 commits September 1, 2020 10:51

Fix trailing new line

b1b2de3

cf [review comment](devfile#122 (comment)) Signed-off-by: David Festal <[email protected]>

Fix formating

6e8a7c8

cf [review comment](devfile#122 (comment)) Signed-off-by: David Festal <[email protected]>

Add documentation to Keyed and related types...

3c18672

... And use `Keyed` instead of `string` as the item type in the `TopLevelLists` type Signed-off-by: David Festal <[email protected]>

Update pkg/utils/overriding/merging.go

eed552e

Co-authored-by: Angel Misevski <[email protected]>

Update pkg/utils/overriding/merging.go

b613251

Co-authored-by: Angel Misevski <[email protected]>

Update pkg/utils/overriding/keys.go

907f8e6

Co-authored-by: Angel Misevski <[email protected]>

Update pkg/utils/overriding/keys.go

b56fb71

Co-authored-by: Angel Misevski <[email protected]>

Fix a non-compiling fix

ba4ff66

Signed-off-by: David Festal <[email protected]>

Fix formatting after accepting a suggestion

af9e127

Signed-off-by: David Festal <[email protected]>

Bring exported functions at the top of the file

ae2615b

according to this [review comment](devfile#122 (comment)) Signed-off-by: David Festal <[email protected]>

Add comments in the function

f5bcbc6

Signed-off-by: David Festal <[email protected]>

Try to simplify the merging function...

88d00ae

... according to the following [review coment](devfile#122 (comment)) Signed-off-by: David Festal <[email protected]>

typo

7b8dbe0

Signed-off-by: David Festal <[email protected]>

Simplify checkKeys function

ef7ba59

Simplify checkKeys function in overriding util by separating out the merging maps step Signed-off-by: Angel Misevski <[email protected]>

amisevsk approved these changes Sep 1, 2020

View reviewed changes

Fix overriding test to account for Go's map traversal order

08efbc1

Fix tests that check error messages to ignore order, since Go iterates through maps in a random order. Signed-off-by: Angel Misevski <[email protected]>

kadel commented Sep 2, 2020

View reviewed changes

davidfestal merged commit 6cb3034 into devfile:master Sep 2, 2020

kadel added a commit to kadel/registry-old that referenced this pull request Sep 9, 2020

update devfiles to match new schema

43e20f6

new schema is introduced in devfile/api#122

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manage merge keys of top-level lists in a more consistent way #122

Manage merge keys of top-level lists in a more consistent way #122

kadel commented Aug 20, 2020 •

edited

Loading

amisevsk commented Aug 20, 2020

amisevsk left a comment

kadel commented Aug 21, 2020

amisevsk commented Aug 21, 2020

kadel commented Aug 24, 2020

davidfestal commented Aug 24, 2020

davidfestal commented Aug 24, 2020 •

edited

Loading

amisevsk commented Aug 25, 2020

davidfestal Aug 25, 2020

davidfestal commented Aug 26, 2020

amisevsk left a comment

amisevsk commented Sep 1, 2020

amisevsk left a comment

kadel left a comment

Manage merge keys of top-level lists in a more consistent way #122

Manage merge keys of top-level lists in a more consistent way #122

Conversation

kadel commented Aug 20, 2020 • edited 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

Docs PR

amisevsk commented Aug 20, 2020

amisevsk left a comment

Choose a reason for hiding this comment

kadel commented Aug 21, 2020

amisevsk commented Aug 21, 2020

kadel commented Aug 24, 2020

davidfestal commented Aug 24, 2020

davidfestal commented Aug 24, 2020 • edited Loading

amisevsk commented Aug 25, 2020

davidfestal Aug 25, 2020

Choose a reason for hiding this comment

davidfestal commented Aug 26, 2020

amisevsk left a comment

Choose a reason for hiding this comment

amisevsk commented Sep 1, 2020

amisevsk left a comment

Choose a reason for hiding this comment

kadel left a comment

Choose a reason for hiding this comment

kadel commented Aug 20, 2020 •

edited

Loading

davidfestal commented Aug 24, 2020 •

edited

Loading