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

Abstracting pod interface in kubernetes plugin to enable easier vendoring #4152

Merged
merged 1 commit into from
May 2, 2017

Conversation

vjsamuel
Copy link
Contributor

When a user creates an Indexer outside of the beats repo, he/she needs to vendor in the k8s repo. This should ideally be unnecessary as the retrieval of Pods is abstracted from the user. This PR aims at adding a wrapper struct Pod which would be passed to Indexers to retrieve metadata.

@vjsamuel vjsamuel force-pushed the abstract_pod_interface branch from d3fb9f4 to dfc223b Compare April 30, 2017 07:16
@elasticmachine
Copy link
Collaborator

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

1 similar comment
@elasticmachine
Copy link
Collaborator

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

@vjsamuel vjsamuel mentioned this pull request Apr 30, 2017
10 tasks
Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

I like the introduction of the interface so each beat is not dependent on the kubernetes API directly.This will make it also possible to potentially change the client without breaking the beats implementations.

Also great that we have now local vendor dir.

@@ -8,6 +8,8 @@ import (
"github.com/elastic/beats/libbeat/common"
"github.com/elastic/beats/libbeat/logp"

"encoding/json"
Copy link
Member

Choose a reason for hiding this comment

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

some goimports fun ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bleh! i hate it when it does that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

@@ -160,187 +160,6 @@
"revisionTime": "2016-09-16T08:04:11Z"
},
{
"checksumSHA1": "K0iEPnt2DZL5/YrrzAQoMnA+9Pc=",
Copy link
Member

Choose a reason for hiding this comment

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

👍

@ruflin
Copy link
Member

ruflin commented May 1, 2017

@exekias Could also please have a look?

@ruflin
Copy link
Member

ruflin commented May 1, 2017

jenkins, test it

@vjsamuel vjsamuel force-pushed the abstract_pod_interface branch from dfc223b to 352f637 Compare May 1, 2017 06:12
@ruflin ruflin added Metricbeat Metricbeat review labels May 1, 2017
Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Great contribution, should make external plugins leaner and fixes vendoring, thanks!

Left 2 minor comments, LGTM once addressed

@@ -20,7 +20,7 @@ import (
"github.com/elastic/beats/filebeat/spooler"

// Add filebeat level processors
_ "github.com/elastic/beats/filebeat/processors/kubernetes"
_ "github.com/elastic/beats/filebeat/processor/kubernetes"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep consistency with libbeat (which uses processors)

Copy link
Member

Choose a reason for hiding this comment

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

@exekias I disagree on this one as we should change it for some time in libbeat to processor. It is processors for historical reasons.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok then, it seems this was agreed some time ago 👍


for _, index := range p.indexers.GetIndexes(pod) {
delete(p.annotationCache.annotations, index)
}
}

func (p *PodWatcher) getPodMeta(pod *corev1.Pod) *Pod {

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please remove the spurious empty lines at the beginning and end of this function?

@vjsamuel vjsamuel force-pushed the abstract_pod_interface branch from 352f637 to bcbae50 Compare May 2, 2017 15:07
@exekias exekias merged commit 530bc0d into elastic:master May 2, 2017
@vjsamuel vjsamuel deleted the abstract_pod_interface branch May 15, 2017 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants