refactor: move discovery and util under package app/customresource to make packages highly cohesive #2104

CatherineF-dev · 2023-06-27T14:51:11Z

What this PR does / why we need it:

How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2103

CatherineF-dev · 2023-06-27T21:38:40Z

Ready for reviewing~

pkg/app/server.go

rexagod · 2023-06-27T22:32:01Z

pkg/customresource/utils.go

 )

 var config *rest.Config
 var currentKubeClient clientset.Interface
 var currentDiscoveryClient *discovery.DiscoveryClient
 var currentDynamicClient *dynamic.DynamicClient

-// CreateKubeClient creates a Kubernetes clientset and a custom resource clientset.
-func CreateKubeClient(apiserver string, kubeconfig string) (clientset.Interface, error) {


I'm not quite sure why this was moved out?

Remove this dependency
v2/pkg/util
└──v2/pkg/customresource

#2103

util should be independent with customresource.

CatherineF-dev · 2023-06-28T00:55:05Z

/hold

I find it still has these newly added dependencies. Will try to remove these as well.

v2/pkg/customresourcestate
├──v2/pkg/metricshandler
├──v2/pkg/options
├──v2/internal/store

pkg/app/discovery.go

CatherineF-dev · 2023-06-28T12:47:34Z

/unhold

Current godepgraph is

v2/pkg/builder/types
├──v2/pkg/metric_generator
├──v2/pkg/options
├──v2/pkg/metrics_store
└──v2/pkg/customresource
v2/pkg/metric_generator
└──v2/pkg/metric
v2/pkg/options
v2/tests/e2e/framework
v2/pkg/builder
├──v2/pkg/builder/types
├──v2/pkg/metric_generator
├──v2/internal/store
├──v2/pkg/customresource
├──v2/pkg/metrics_store
└──v2/pkg/options
v2/pkg/customresource
└──v2/pkg/metric_generator
v2/pkg/customresourcestate
├──v2/pkg/metric
├──v2/pkg/metric_generator
├──v2/internal/discovery
├──v2/pkg/customresource
└──v2/internal/store
v2/pkg/watch
v2/pkg/sharding
v2/pkg/util/proc
v2/internal
├──v2/pkg/options
└──v2/pkg/app
v2/internal/store
├──v2/pkg/metrics_store
├──v2/pkg/sharding
├──v2/pkg/watch
├──v2/pkg/metric_generator
├──v2/pkg/builder/types
├──v2/pkg/constant
├──v2/pkg/customresource
├──v2/pkg/metric
└──v2/pkg/options
v2/pkg/allow
v2/pkg/allowdenylist
└──v2/pkg/metric_generator
v2/pkg/metricshandler
├──v2/pkg/options
├──v2/pkg/metrics_store
└──v2/pkg/builder/types
v2/pkg/optin
└──v2/pkg/metric_generator
v2
├──v2/internal
└──v2/pkg/options
v2/internal/discovery
v2/pkg/app
├──v2/pkg/optin
├──v2/pkg/customresource
├──v2/pkg/allowdenylist
├──v2/pkg/util/proc
├──v2/pkg/customresourcestate
├──v2/pkg/options
├──v2/pkg/metric_generator
├──v2/internal/store
├──v2/internal/discovery
└──v2/pkg/metricshandler
v2/pkg/metric
v2/pkg/metrics_store/metricsstore
└──v2/pkg/metric
v2/tools

Before: #2103

dgrisonnet · 2023-06-29T16:48:18Z

/triage accepted
/assign @rexagod @mrueg

CatherineF-dev · 2023-07-06T22:54:23Z

Ping ~ @rexagod @mrueg

rexagod · 2023-07-07T11:46:21Z

Sorry for the delay here, I got caught up, will review before end of the day. 🙂

rexagod · 2023-07-08T14:16:40Z

internal/discovery/types.go

@@ -15,93 +15,22 @@ package discovery



Can we move the entire discovery package (what's left of it in internal/discovery under pkg/app as well)? It would make sense to have the types for discovery in the same place as the logic.

This would also help clean up the same import from app/discovery.go and app/discovery_test.go.

If moving entire discovery package, v2/pkg/customresourcestate will depend on all packages where internal/discovery depends on.

This is shown in #2103

ACK, can we move the discovery types inside pkg/app to it's own package (could be types, which would contain all such exported types for the app package, since discovery comes under that now)?

EDIT: It seems moving them under pkg would make more sense, since these are used in areas other than just pkg/app.

discovery.go and internal/discovery/types.go should be in different packages. Because customresourcestate depends on internal/discovery/types.go and discovery.go depends on many other pkgs.

Does can we move the discovery types inside pkg/app to it's own package mean moving internal/discovery/types.go under types and discovery.go under app?

My bad, it seems the discovery types are not used in internal, so moving these types within a types package in pkg is a valid alternative, in case we do not want to have a root-level types package.

I'm not sure if it's worth reorganizing when we want to change the way customresource metrics are created at the same time, so I don't have a strong opinion here.

ACK. In that case I think we can proceed with a types package within pkg that encapsulates all types used throughout pkg, instead of a root-level types package. This would ensure relevant types live close to their logic, while providing a degree of desired isolation away from the package the logic is present under.

@CatherineF-dev Does this sound good?

QQ: what's the package name for types.go? pkg or types.

types should suffice I believe.

pkg/app/server.go

pkg/app/discovery.go

pkg/customresourcestate/config.go

internal/discovery/types.go

k8s-ci-robot · 2023-07-08T15:16:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: CatherineF-dev
Once this PR has been reviewed and has the lgtm label, please ask for approval from mrueg. 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

pkg/customresourcestate/config.go

use interface to decouple customresourcestate and CRDiscoverer

Update pkg/app/discovery.go Co-authored-by: Pranshu Srivastava <[email protected]> Update internal/discovery/types.go Co-authored-by: Pranshu Srivastava <[email protected]> Update internal/discovery/types.go Co-authored-by: Pranshu Srivastava <[email protected]> Update pkg/customresourcestate/config.go Co-authored-by: Pranshu Srivastava <[email protected]> Update pkg/customresourcestate/config.go Co-authored-by: Pranshu Srivastava <[email protected]>

k8s-triage-robot · 2024-01-21T22:20:51Z

The Kubernetes project currently lacks enough contributors to adequately respond to all 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:

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

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

/lifecycle stale

k8s-triage-robot · 2024-02-20T22:41:31Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all 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:

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

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

/lifecycle rotten

k8s-triage-robot · 2024-03-26T20:16:52Z

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 · 2024-03-26T20:16:58Z

@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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 27, 2023

k8s-ci-robot requested review from logicalhan and rexagod June 27, 2023 14:51

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 27, 2023

CatherineF-dev changed the title ~~Move discovery and util under package customresourcestate to make package higly cohesive~~ [WIP] Move discovery and util under package customresourcestate to make package higly cohesive Jun 27, 2023

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 27, 2023

CatherineF-dev changed the title ~~[WIP] Move discovery and util under package customresourcestate to make package higly cohesive~~ [WIP] Move discovery and util under package customresourcestate to make packages higly cohesive Jun 27, 2023

CatherineF-dev changed the title ~~[WIP] Move discovery and util under package customresourcestate to make packages higly cohesive~~ [WIP] Move discovery and util under package customresourcestate to make packages highly cohesive Jun 27, 2023

CatherineF-dev force-pushed the refactor-to-be-high-cohension branch from 6972a72 to 7ff1ff4 Compare June 27, 2023 20:59

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 27, 2023

CatherineF-dev changed the title ~~[WIP] Move discovery and util under package customresourcestate to make packages highly cohesive~~ Move discovery and util under package customresourcestate/customresource to make packages highly cohesive Jun 27, 2023

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 Jun 27, 2023

CatherineF-dev changed the title ~~Move discovery and util under package customresourcestate/customresource to make packages highly cohesive~~ refactor: move discovery and util under package customresourcestate/customresource to make packages highly cohesive Jun 27, 2023

CatherineF-dev force-pushed the refactor-to-be-high-cohension branch 2 times, most recently from 5450836 to 18f03c1 Compare June 27, 2023 21:29

logicalhan reviewed Jun 27, 2023

View reviewed changes

pkg/app/server.go Outdated Show resolved Hide resolved

pkg/app/server.go Outdated Show resolved Hide resolved

rexagod reviewed Jun 27, 2023

View reviewed changes

CatherineF-dev force-pushed the refactor-to-be-high-cohension branch from c721bf0 to b6a8e0d Compare June 27, 2023 23:57

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 28, 2023

CatherineF-dev force-pushed the refactor-to-be-high-cohension branch 3 times, most recently from e6a46fe to ab98c78 Compare June 28, 2023 12:10

mrueg reviewed Jun 28, 2023

View reviewed changes

pkg/app/discovery.go Show resolved Hide resolved

CatherineF-dev force-pushed the refactor-to-be-high-cohension branch from ea97350 to e67b6c8 Compare June 28, 2023 12:43

CatherineF-dev changed the title ~~refactor: move discovery and util under package customresourcestate/customresource to make packages highly cohesive~~ refactor: move discovery and util under package app/customresource to make packages highly cohesive Jun 28, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 28, 2023

k8s-ci-robot assigned mrueg Jun 29, 2023

k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jun 29, 2023

k8s-ci-robot assigned rexagod Jun 29, 2023

k8s-ci-robot removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jun 29, 2023

rexagod requested changes Jul 8, 2023

View reviewed changes

rexagod reviewed Jul 9, 2023

View reviewed changes

pkg/customresourcestate/config.go Show resolved Hide resolved

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

CatherineF-dev and others added 2 commits September 26, 2023 17:20

mv discovery to app, util to customresource

fff8c1a

use interface to decouple customresourcestate and CRDiscoverer

CatherineF-dev force-pushed the refactor-to-be-high-cohension branch from 2bbd48e to e134ac9 Compare September 26, 2023 21:21

move to util.go

1ff59e3

CatherineF-dev force-pushed the refactor-to-be-high-cohension branch from b41d1ff to 1ff59e3 Compare September 26, 2023 21:49

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

rexagod mentioned this pull request Jan 3, 2024

refactor: make ResolveGVKToGVKPs shorter and use if-else #2271

Closed

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

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 Feb 20, 2024

k8s-ci-robot closed this Mar 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move discovery and util under package app/customresource to make packages highly cohesive #2104

refactor: move discovery and util under package app/customresource to make packages highly cohesive #2104

CatherineF-dev commented Jun 27, 2023

CatherineF-dev commented Jun 27, 2023

rexagod Jun 27, 2023 •

edited

Loading

CatherineF-dev Jun 27, 2023 •

edited

Loading

CatherineF-dev commented Jun 28, 2023

CatherineF-dev commented Jun 28, 2023 •

edited

Loading

dgrisonnet commented Jun 29, 2023

CatherineF-dev commented Jul 6, 2023

rexagod commented Jul 7, 2023

rexagod Jul 8, 2023

rexagod Jul 8, 2023

CatherineF-dev Jul 9, 2023 •

edited

Loading

rexagod Jul 9, 2023 •

edited

Loading

CatherineF-dev Jul 9, 2023

rexagod Jul 9, 2023

mrueg Jul 13, 2023

rexagod Jul 13, 2023

CatherineF-dev Sep 26, 2023

rexagod Feb 25, 2024 •

edited

Loading

k8s-ci-robot commented Jul 8, 2023

k8s-triage-robot commented Jan 21, 2024

k8s-triage-robot commented Feb 20, 2024

k8s-triage-robot commented Mar 26, 2024

k8s-ci-robot commented Mar 26, 2024

refactor: move discovery and util under package app/customresource to make packages highly cohesive #2104

refactor: move discovery and util under package app/customresource to make packages highly cohesive #2104

Conversation

CatherineF-dev commented Jun 27, 2023

CatherineF-dev commented Jun 27, 2023

rexagod Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

CatherineF-dev Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

CatherineF-dev commented Jun 28, 2023

CatherineF-dev commented Jun 28, 2023 • edited Loading

dgrisonnet commented Jun 29, 2023

CatherineF-dev commented Jul 6, 2023

rexagod commented Jul 7, 2023

rexagod Jul 8, 2023

Choose a reason for hiding this comment

rexagod Jul 8, 2023

Choose a reason for hiding this comment

CatherineF-dev Jul 9, 2023 • edited Loading

Choose a reason for hiding this comment

rexagod Jul 9, 2023 • edited Loading

Choose a reason for hiding this comment

CatherineF-dev Jul 9, 2023

Choose a reason for hiding this comment

rexagod Jul 9, 2023

Choose a reason for hiding this comment

mrueg Jul 13, 2023

Choose a reason for hiding this comment

rexagod Jul 13, 2023

Choose a reason for hiding this comment

CatherineF-dev Sep 26, 2023

Choose a reason for hiding this comment

rexagod Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 8, 2023

k8s-triage-robot commented Jan 21, 2024

k8s-triage-robot commented Feb 20, 2024

k8s-triage-robot commented Mar 26, 2024

k8s-ci-robot commented Mar 26, 2024

rexagod Jun 27, 2023 •

edited

Loading

CatherineF-dev Jun 27, 2023 •

edited

Loading

CatherineF-dev commented Jun 28, 2023 •

edited

Loading

CatherineF-dev Jul 9, 2023 •

edited

Loading

rexagod Jul 9, 2023 •

edited

Loading

rexagod Feb 25, 2024 •

edited

Loading