Refactor Inventory client #586

rquitales · 2022-06-03T16:11:56Z

This is a WIP PR pending rebase when K8s resources are updated to use v1.25.

This PR uses Karl's initial PR as the basis for the Inventory client refactor.

Rebased Karl's code onto cli-utils tip
Updated deepcopy-gen to also create DeepCopyObject for actuation.Inventory
Remove stuttering on certain Inventory objects and methods after rebase

TODO: Rebase PR when blocking PRs are merged

linux-foundation-easycla · 2022-06-03T16:12:00Z

The committers listed above are authorized under a signed CLA.

✅ login: rquitales / name: Ramon Quitales (e952671, 3818d4b, 44b66cc, b002832, 7b18e13, 6f36891)

k8s-ci-robot · 2022-06-03T16:12:04Z

Hi @rquitales. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

janetkuo · 2022-06-03T20:34:47Z

/ok-to-test

Makefile

karlkfi · 2022-06-03T21:03:33Z

pkg/apis/actuation/types.go

@@ -8,6 +8,8 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 )

+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object


is this needed for this PR? It can probably be pulled out.

The refactor now requires Inventory to satisfy runtime.Object which was previously not needed, hence why deepcopy-gen tags are added in this PR. We could spin it out into another PR that is merged first, but this tag addition is related to the refactor. LMKYT

I agree it's required for this PR, but it doesn't need to be IN this PR. The dependency only goes one way. So I think it can go in a separate PR just to reduce the size of this PR.

pkg/inventory/client-factory.go

k8s-triage-robot · 2022-09-26T19:33:38Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

karlkfi · 2022-10-11T23:12:52Z

cmd/main.go

@@ -62,7 +62,8 @@ func main() {
 	}

 	loader := manifestreader.NewManifestLoader(f)
-	invFactory := inventory.ClusterClientFactory{StatusPolicy: inventory.StatusPolicyNone}
+	// TODO: Add StatusPolicy back to factory.


Does ConfigMapClientFactory need configurable StatusPolicy? Or is it configured elsewhere?

ConfigMapClientFactory does need configurable StatusPolicy. We have historically hardcoded StatusPolicyNone for kapply as the statusPolicy field is not exposed as a flag. Perhaps we should also add a flag to kapply to have users set this in a follow-up PR?

Regardless, ConfigMap now has a configurable StatusPolicy field for handling status conversion to the data fields.

cmd/status/cmdstatus.go

k8s-ci-robot · 2022-11-03T16:58:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rquitales
Once this PR has been reviewed and has the lgtm label, please assign karlkfi for approval by writing /assign @karlkfi in a comment. For more information see the Kubernetes Code Review Process.

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

This commit uses Karl's inventory client changes and is a direct c&p from his PR. This is a WIP commit and further commits need to be created for kapply status to work as intended.

k8s-triage-robot · 2022-12-09T21:02:22Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

karlkfi · 2022-12-20T23:11:39Z

pkg/apply/task/inv_set_task_test.go

@@ -164,18 +166,25 @@ func TestInvSetTask(t *testing.T) {

 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
-			client := inventory.NewFakeClient(object.ObjMetadataSet{})
+			// client := inventory.NewFakeInventoryClient(object.ObjMetadataSet{})


remove commented line

karlkfi · 2022-12-20T23:19:43Z

test/e2e/e2e_test.go

@@ -29,7 +29,7 @@ const (

 var inventoryConfigs = map[string]invconfig.InventoryConfig{}
 var inventoryConfigTypes = []string{
-	ConfigMapTypeInvConfig,
+	// ConfigMapTypeInvConfig,


why commented out?

karlkfi

I've identified a bunch of code that looks independent to the core of this PR (to rewrite the inventory client). They don't ALL need to be pulled out, and they don't ALL need to be individual PRs. Feel free to group them at your own discretion. Then link them from this PR and assign them to me. After they're merged, you can rebase this PR.

karlkfi · 2022-12-22T00:46:28Z

cmd/apply/cmdapply.go

@@ -129,7 +129,7 @@ func (r *Runner) RunE(cmd *cobra.Command, args []string) error {
 	if err != nil {
 		return err
 	}
-	inv := inventory.WrapInventoryInfoObj(invObj)
+	inv := inventory.InfoFromObject(invObj)


Please move this WrapInventoryInfoObj -> InfoFromObject rename out to its own PR to reduce noise.

Feel free to include other renames that aren't critical to the rewrite, like inv -> invInfo.

karlkfi · 2022-12-22T00:47:41Z

cmd/main.go

@@ -62,7 +62,7 @@ func main() {
 	}

 	loader := manifestreader.NewManifestLoader(f)
-	invFactory := inventory.ClusterClientFactory{StatusPolicy: inventory.StatusPolicyNone}
+	invFactory := inventory.ConfigMapClientFactory{StatusPolicy: inventory.StatusPolicyNone}


This ClusterClientFactory -> ConfigMapClientFactory rename can be moved out to the refactor/rename PR to reduce noise.

karlkfi · 2022-12-22T00:48:18Z

cmd/status/cmdstatus.go

@@ -152,7 +152,7 @@ func (r *Runner) preRunE(*cobra.Command, []string) error {
 // and get info from the cluster based on the local info
 // wrap it to be a map mapping from string to objectMetadataSet
 func (r *Runner) loadInvFromDisk(cmd *cobra.Command, args []string) (*printer.PrintData, error) {
-	inv, err := r.loader.GetInvInfo(cmd, args)
+	invInfo, err := r.loader.GetInvInfo(cmd, args)


This inv -> invInfo refactor variable can be moved out to a smaller PR to reduce noise here.

karlkfi · 2022-12-22T00:51:38Z

pkg/apis/actuation/types.go

@@ -8,6 +8,8 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 )

+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object


I agree it's required for this PR, but it doesn't need to be IN this PR. The dependency only goes one way. So I think it can go in a separate PR just to reduce the size of this PR.

karlkfi · 2022-12-22T00:52:28Z

pkg/apply/applier.go

-	for _, localObj := range localObjs {
-		inventory.AddInventoryIDAnnotation(localObj, localInv)
+	for _, applyObj := range applyObjs {
+		inventory.SetOwningInventoryAnnotation(applyObj, invInfo.ID)


This rename can be pulled out to the smaller refactor PR: AddInventoryIDAnnotation -> SetOwningInventoryAnnotation

karlkfi · 2022-12-22T01:25:51Z

pkg/inventory/fake-client-factory.go

+	"sigs.k8s.io/cli-utils/pkg/object"
+)
+
+type FakeClientFactory object.ObjMetadataSet


public types need comments. Not sure why the linter didn't catch this.

karlkfi · 2022-12-22T01:28:33Z

pkg/inventory/inventory_error.go

 type NoInventoryObjError struct{}

-func (e *NoInventoryObjError) Error() string {
+func (g NoInventoryObjError) Error() string {


Generally, error types should use a pointer receiver, to allow them to be nil, even if there's no state that actually needs a pointer. Making the Error method require a pointer is a good way to force users to follow this best practice when using it.

karlkfi · 2022-12-22T01:29:48Z

pkg/inventory/inventory_test.go

-			}
-		}
+		actual := Label(object.InfoToUnstructured(test.inventoryInfo))
+		assert.Equal(t, test.inventoryLabel, actual)


Here's another assert.Equal test cleanup that could be pulled out.

karlkfi · 2022-12-22T01:31:25Z

pkg/inventory/main_test.go

@@ -0,0 +1,19 @@
+// Copyright 2022 The Kubernetes Authors.


This test file could be pulled out. It's not a big deal if there's only one, but it's completely independent. It just allows the package to be run with logging verbosity.

karlkfi · 2022-12-22T01:32:02Z

pkg/inventory/type-conv.go

 )

+// ObjMetadataEqualObjectReference compares an ObjMetadata with a ObjectReference
+func ObjMetadataEqualObjectReference(id object.ObjMetadata, ref actuation.ObjectReference) bool {


This helper function looks pretty independent. It could be in a separate PR.

In fact, this whole file seems like helper funcs that could be factored into a separate PR.

k8s-triage-robot · 2023-01-21T02:16:59Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2023-01-21T02:17:04Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 3, 2022

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 3, 2022

k8s-ci-robot requested review from karlkfi and Liujingfang1 June 3, 2022 16:12

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 3, 2022

rquitales force-pushed the rquitales-inventory-client branch from 49e276f to 6f36891 Compare June 3, 2022 20:24

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 3, 2022

karlkfi reviewed Jun 3, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

karlkfi reviewed Jun 3, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

karlkfi reviewed Jun 3, 2022

View reviewed changes

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 3, 2022

rquitales force-pushed the rquitales-inventory-client branch from 6f36891 to f995c77 Compare June 6, 2022 22:13

janetkuo reviewed Jun 6, 2022

View reviewed changes

pkg/inventory/client-factory.go Outdated Show resolved Hide resolved

rquitales force-pushed the rquitales-inventory-client branch 3 times, most recently from 08b3615 to 3ad0f74 Compare June 7, 2022 22:05

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 8, 2022

rquitales force-pushed the rquitales-inventory-client branch 2 times, most recently from 80e460d to 7c5fef1 Compare June 9, 2022 17:58

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2022

rquitales force-pushed the rquitales-inventory-client branch 4 times, most recently from f44bea1 to f5e7d89 Compare June 9, 2022 22:13

rquitales force-pushed the rquitales-inventory-client branch from a60cabe to b10923d Compare June 15, 2022 20:58

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 26, 2022

rquitales force-pushed the rquitales-inventory-client branch from 983f54d to c1ef322 Compare September 29, 2022 19:43

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2022

karlkfi reviewed Oct 11, 2022

View reviewed changes

cmd/status/cmdstatus.go Outdated Show resolved Hide resolved

rquitales force-pushed the rquitales-inventory-client branch from 1c02add to 69dc8af Compare November 3, 2022 16:58

rquitales force-pushed the rquitales-inventory-client branch 2 times, most recently from 9cf3cde to 5c8df4f Compare November 3, 2022 21:47

rquitales added 6 commits November 3, 2022 21:50

refactor: Update inventory client

ea0c81f

This commit uses Karl's inventory client changes and is a direct c&p from his PR. This is a WIP commit and further commits need to be created for kapply status to work as intended.

fix: Update kapply status to work with new client

fd6e3ff

fix: inject mapper into solver tests

de2ca7c

fix: add status policyall to e2e test

2b081bf

chore: go fmt

af418cd

fix: Re-add StatusPolicy handling for ConfigMaps

819cb6e

rquitales force-pushed the rquitales-inventory-client branch from 5c8df4f to 819cb6e Compare November 3, 2022 21:50

rquitales changed the title ~~[WIP] Refactor Inventory client~~ Refactor Inventory client Nov 9, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 9, 2022

karlkfi reviewed Dec 20, 2022

View reviewed changes

karlkfi requested changes Dec 22, 2022

View reviewed changes

k8s-ci-robot closed this Jan 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Inventory client #586

Refactor Inventory client #586

rquitales commented Jun 3, 2022 •

edited

linux-foundation-easycla bot commented Jun 3, 2022 •

edited

k8s-ci-robot commented Jun 3, 2022

janetkuo commented Jun 3, 2022

karlkfi Jun 3, 2022

rquitales Nov 3, 2022

karlkfi Dec 22, 2022

k8s-triage-robot commented Sep 26, 2022

karlkfi Oct 11, 2022

rquitales Nov 3, 2022 •

edited

k8s-ci-robot commented Nov 3, 2022

k8s-triage-robot commented Dec 9, 2022

karlkfi Dec 20, 2022

karlkfi Dec 20, 2022

karlkfi left a comment

karlkfi Dec 22, 2022

karlkfi Dec 22, 2022

karlkfi Dec 22, 2022

karlkfi Dec 22, 2022

karlkfi Dec 22, 2022

karlkfi Dec 22, 2022

karlkfi Dec 22, 2022

karlkfi Dec 22, 2022

karlkfi Dec 22, 2022

karlkfi Dec 22, 2022

k8s-triage-robot commented Jan 21, 2023

k8s-ci-robot commented Jan 21, 2023

Refactor Inventory client #586

Refactor Inventory client #586

Conversation

rquitales commented Jun 3, 2022 • edited

linux-foundation-easycla bot commented Jun 3, 2022 • edited

k8s-ci-robot commented Jun 3, 2022

janetkuo commented Jun 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-triage-robot commented Sep 26, 2022

Choose a reason for hiding this comment

rquitales Nov 3, 2022 • edited

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 3, 2022

k8s-triage-robot commented Dec 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlkfi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-triage-robot commented Jan 21, 2023

k8s-ci-robot commented Jan 21, 2023

rquitales commented Jun 3, 2022 •

edited

linux-foundation-easycla bot commented Jun 3, 2022 •

edited

rquitales Nov 3, 2022 •

edited