Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove collector package #786

Merged
merged 3 commits into from
Jun 11, 2019
Merged

Conversation

lilic
Copy link
Member

@lilic lilic commented Jun 11, 2019

What this PR does / why we need it:

Initially, this started because we noticed, while using kube-state-metrics as a library, that there is an unnecessary pointer to an interface. Wanted to remove that pointer, but noticed that the collector package does not need to exist, as it's just a wrapper around store and instead we can just do a call .WriteAll(w) on the store object directly.

The majority of this PR is renaming things from collector -> store.

Note: None of the user-facing code was changed, as that would be a breaking change. So logs, flags that mention collectors stay unchanged. IMHO user-facing "collectors" flag should be renamed to "resources", but like I said that would be a user breaking change.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 11, 2019
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 11, 2019
Copy link
Member

@brancz brancz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are any of the vendoring changes necessary? It seems like if those are just updated, then we should do that in a separate PR.

Overall looks good. Happy to get rid of that effectively unused package.

@@ -0,0 +1,264 @@
/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any way we can preserve the history of this one? it's really just a rename right?

Copy link
Member Author

@lilic lilic Jun 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not just a file rename, it also renames the individual build functions to Store suffix, e.g. buildDaemonSetCollection -> buildDaemonSetStore

guthub is bad at displaying the diff, but I split it multiple commits, so hopefully, it's easier. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see. In that case the diff is probably too large for git to acknowledge, that this might be a rename, so it would have had to happen in two commits. In that case I'm ok with it as is.

@lilic
Copy link
Member Author

lilic commented Jun 11, 2019

Are any of the vendoring changes necessary? It seems like if those are just updated, then we should do that in a separate PR.

I was wondering this as well when I noticed the big diff of added files. I think I ran the go mod vendor in case any of the deps in the collector package were removed, but seems like that triggered this update. Will remove this locally and test.

@lilic
Copy link
Member Author

lilic commented Jun 11, 2019

@brancz Update: the changes in the vendor directly are the result of running go mod vendor, as there is a check for that in validate-modules target, hence the change to the vendor dir.

collector package was an unnecessary wrapper. This replaces the
.Collect with a call to Store.WriteAll every time a collect is needed.
Since the removal of collector, this introduces both the concept of the
store and the resources instead of collectors that the user passes in.

The user facing logs and flags were not changed as that would be a
regression.
@brancz
Copy link
Member

brancz commented Jun 11, 2019

Interesting. In that case this lgtm. Thanks!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 11, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, LiliC

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 11, 2019
@lilic
Copy link
Member Author

lilic commented Jun 11, 2019

Travis seems to be having some issues with starting the build. 🤷‍♀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants