Implement feature management policy and reference #21

morningspace · 2022-01-04T15:08:35Z

Signed-off-by: Ying Mo morningspace@yahoo.com

Description of your changes

Fixes #13 to implement the proposed features that are documented in this design doc.

I have:

Read and followed Crossplane's contribution process.
Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

All newly-added methods have been tested by adding new test cases in object_test.go. Just follow the same pattern that is used on object_test.go to define sample data, add new cases with expected results, and iterate over the cases to trigger Observe/Create/Update/Delete, also AddFinalizer/RemoveFinalizer. The code coverage has also been increased.

Signed-off-by: Ying Mo <morningspace@yahoo.com>

morningspace · 2022-01-04T15:13:56Z

Hi @turkenh This is the initial commit for an early review. The feature has been manually tested at local and I was adding test cases to catch up the code coverage, will submit a second commit including the test cases soon.

Signed-off-by: Ying Mo <morningspace@yahoo.com>

morningspace · 2022-01-05T06:58:03Z

Added test cases and now the coverage has been increased to 81.17% :-) https://codecov.io/github/crossplane-contrib/provider-kubernetes/commit/d7bcc5a8e1aff6bad593147b0f2b95fd4af31405

turkenh

Thanks @morningspace, added couple of comments mostly around styling and nitpick.

I just have some questions for the client to use reference resolution and also a concern with ignoring errors.

apis/object/v1alpha1/types.go

internal/controller/object/object.go

Signed-off-by: Ying Mo <morningspace@yahoo.com>

morningspace · 2022-01-08T04:40:42Z

Code has been updated according to the review comments. Now, all newly added code has been fully covered by unit tests. :-) @turkenh

turkenh

Looking pretty good @morningspace, I think we are very close to get this merged.

I did some testing this time using the manifest I used in some of my comments and working well except minor issues I've commented.

Could you also include some examples under examples/object and fill the How has this code been tested section in PR description :)

Thanks again also for your patience during this review process!

apis/object/v1alpha1/types.go

internal/controller/object/object.go

apis/object/v1alpha1/types.go

turkenh · 2022-01-20T11:29:21Z

internal/controller/object/object.go

+	err = f.handleRefFinalizer(ctx, obj, func(
+		ctx context.Context, res *unstructured.Unstructured, finalizer string) error {
+		if !meta.FinalizerExists(res, finalizer) {
+			meta.AddFinalizer(res, finalizer)


One problem with adding finalizers to referred resources is how to handle changes there. For example, an Object depends on secret-a, but then changed to secret-b with a kubectl apply, in that case, we would need to remove finalizer from secret-a and add secret-b.
However, this could be handled with a subsequent PR, feel free to create an issue to track such a case and ignore for this PR.

That makes sense. Issue created #28.

apis/object/v1alpha1/types.go

haarchri · 2022-02-14T07:00:32Z

@morningspace can you check latest comments ? We really want to use this feature ;)

morningspace · 2022-02-14T13:04:31Z

Hey @haarchri, also @turkenh, sorry for my slowness! Just want to let you know that I was stuck into some project work last week, and just resume this work yesterday. I plan to finish the code change this week that will address the last couple of review comments made by @turkenh.

Signed-off-by: Ying Mo <morningspace@yahoo.com>

…-kubernetes into feature-mgmtpolicy-reference

…eam change Signed-off-by: Ying Mo <morningspace@yahoo.com>

Signed-off-by: Ying Mo <morningspace@yahoo.com>

morningspace · 2022-02-16T03:48:39Z

Alright, I think ultimately I've made all changes needed per latest comments. @turkenh Could you please take another round of review, and see if anything missed :-)

turkenh

Looking great, thanks for incorporating all the feedback @morningspace 👍

My only suggestion would be to consider some sort of documentation, especially how to configure RBAC if other resources need to be referenced. Your design doc is already great for anyone that wants to understand how things are working (which also needs some updates due to naming changes during implementation), but a simpler document or maybe a small section in Readme could be helpful. But this is definitely not blocking, please let me know if you want to handle this separately so that we can merge this!

morningspace · 2022-02-18T06:04:57Z

@turkenh wow, it's great to see the PR gets approved ultimately!

Re: docs updates, sure, how about having a separate PR to address it since this PR has been full of so many comments and makes it hard to follow. I will start another PR to cover all docs related work.

cdenneen · 2022-02-25T14:29:33Z

Instead of the patchesFrom being another object could it be from a patchSet? I’m trying to patch aws-auth which mapRoles is a Literal Scalar Block

morningspace · 2022-02-25T23:31:12Z

@cdenneen IIUC, you are referring to the patchSet in Composition right? So, we are kind of copying stuff from Composition to provider level. I'd defer it till we see there's no other better option. Can you open a separate issue on this and share a concrete example w/ us? I'd like to better understand the problem and see if there's better option. Thanks.

init commit for feature mgmt policy and reference

ef88fd6

Signed-off-by: Ying Mo <morningspace@yahoo.com>

morningspace changed the title ~~init commit for feature mgmt policy and reference~~ Implement feature management policy and reference Jan 4, 2022

add test cases

e8ea770

Signed-off-by: Ying Mo <morningspace@yahoo.com>

morningspace force-pushed the feature-mgmtpolicy-reference branch from d7bcc5a to e8ea770 Compare January 5, 2022 06:53

turkenh reviewed Jan 6, 2022

View reviewed changes

morningspace added 3 commits January 8, 2022 09:52

refine per review comments

7287e6b

Signed-off-by: Ying Mo <morningspace@yahoo.com>

fix lint errors

ed3790b

Signed-off-by: Ying Mo <morningspace@yahoo.com>

small refinements

c753317

Signed-off-by: Ying Mo <morningspace@yahoo.com>

turkenh reviewed Jan 20, 2022

View reviewed changes

stewartshea mentioned this pull request Jan 20, 2022

secretKeyRef overwrites key name crossplane-contrib/provider-helm#118

Open

morningspace added 3 commits February 15, 2022 16:10

Use dependsOn & patchesFrom and default to Object

57ab7bd

Signed-off-by: Ying Mo <morningspace@yahoo.com>

Fix lint error

f4c30d5

Signed-off-by: Ying Mo <morningspace@yahoo.com>

Append spec.forProvider.manifest. to toFieldPath

ccfa086

Signed-off-by: Ying Mo <morningspace@yahoo.com>

morningspace mentioned this pull request Feb 15, 2022

Handle finalizer for referred resource properly when referred resource is changed #28

Open

morningspace added 2 commits February 15, 2022 17:35

Merge branch 'main' of https://github.com/crossplane-contrib/provider…

344efd2

…-kubernetes into feature-mgmtpolicy-reference

Update kubernetes.crossplane.io_providerconfigs.yaml to reflect upstr…

6a2499b

…eam change Signed-off-by: Ying Mo <morningspace@yahoo.com>

morningspace force-pushed the feature-mgmtpolicy-reference branch from 5bba4ec to 6a2499b Compare February 15, 2022 09:51

morningspace added 2 commits February 16, 2022 11:24

Refactor getReferenceInfo

19f1de7

Signed-off-by: Ying Mo <morningspace@yahoo.com>

Add examples

46ef0d7

Signed-off-by: Ying Mo <morningspace@yahoo.com>

morningspace mentioned this pull request Feb 16, 2022

Patch existing resources(Object) #26

Closed

turkenh approved these changes Feb 17, 2022

View reviewed changes

turkenh merged commit 9bfb526 into crossplane-contrib:main Feb 18, 2022

morningspace mentioned this pull request Feb 24, 2022

Dependencies between resources #12

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement feature management policy and reference #21

Implement feature management policy and reference #21

morningspace commented Jan 4, 2022 •

edited

morningspace commented Jan 4, 2022

morningspace commented Jan 5, 2022 •

edited

turkenh left a comment

morningspace commented Jan 8, 2022

turkenh left a comment

turkenh Jan 20, 2022

morningspace Feb 15, 2022

haarchri commented Feb 14, 2022

morningspace commented Feb 14, 2022

morningspace commented Feb 16, 2022

turkenh left a comment

morningspace commented Feb 18, 2022

cdenneen commented Feb 25, 2022

morningspace commented Feb 25, 2022

Implement feature management policy and reference #21

Implement feature management policy and reference #21

Conversation

morningspace commented Jan 4, 2022 • edited

Description of your changes

How has this code been tested

morningspace commented Jan 4, 2022

morningspace commented Jan 5, 2022 • edited

turkenh left a comment

Choose a reason for hiding this comment

morningspace commented Jan 8, 2022

turkenh left a comment

Choose a reason for hiding this comment

turkenh Jan 20, 2022

Choose a reason for hiding this comment

morningspace Feb 15, 2022

Choose a reason for hiding this comment

haarchri commented Feb 14, 2022

morningspace commented Feb 14, 2022

morningspace commented Feb 16, 2022

turkenh left a comment

Choose a reason for hiding this comment

morningspace commented Feb 18, 2022

cdenneen commented Feb 25, 2022

morningspace commented Feb 25, 2022

morningspace commented Jan 4, 2022 •

edited

morningspace commented Jan 5, 2022 •

edited