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

Metricbeat : new metricset image for docker module #3467

Merged
merged 14 commits into from
Feb 3, 2017

Conversation

douaejeouit
Copy link
Contributor

Basic informations about the images available on the server

@karmi
Copy link

karmi commented Jan 25, 2017

Hi @douaejeouit, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@douaejeouit
Copy link
Contributor Author

@karmi it's done!
Thanks

@douaejeouit
Copy link
Contributor Author

@ruflin your review is welcome!
thanks

@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.

@karmi
Copy link

karmi commented Jan 25, 2017

@douaejeouit thanks!, but it keeps failing, have you added your Gmail to the Github profile? That should normally suffice to make that green, even when the commit has a different e-mail.

@douaejeouit
Copy link
Contributor Author

@karmi that's weird 😢 I added both of my e-mails ..

@karmi
Copy link

karmi commented Jan 26, 2017

Hi @douaejeouit, I've reached out via e-mail, ping me if you need assistance!

Copy link
Contributor

@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.

Some notes

  • It seems the local fields.yml for the metricset is missing
  • Could you generate the data.json?
  • Can you add a docs check test to test_docker.py?
  • Update to CHANGELOG.asciidoc

This should also make the build green. Don't forget to run make update and make fmt.

update templates
add data.json
add docs check to test_docker.py
update CHANGELOG.asciidoc
make update - make fmt - make collect
output = self.read_output_json()
evt = output[0]

evt = self.remove_labels(evt)
Copy link
Contributor

Choose a reason for hiding this comment

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

As the image data set probably doesn't have fields, this line is not needed

},
"repoTags": image.RepoTags,
}
labels := docker.DeDotLabels(image.Labels)
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see it has labels. Ignore my comment below. But it seems the line with the labels somehow breaks the tests. See Travis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll check it!

- name: example
type: keyword
description: >
Example field
Copy link
Contributor

Choose a reason for hiding this comment

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

This must be completed with the fields, otherwise the system test will not pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll complete it!

@douaejeouit
Copy link
Contributor Author

@ruflin Have you thought about the way we should document lists?

@ruflin
Copy link
Contributor

ruflin commented Feb 2, 2017

@douaejeouit Which lists are you referring to? labels? My solution currently is to not document it (which is not great). Perhaps we could at least define an empty object: https://www.elastic.co/guide/en/elasticsearch/reference/current/object.html

@ruflin
Copy link
Contributor

ruflin commented Feb 2, 2017

@douaejeouit I found that we use dict-type: keyword here which could become useful: https://github.com/elastic/beats/blob/master/winlogbeat/_meta/fields.yml#L40 But currently I think it does not work with our generation script.

@douaejeouit
Copy link
Contributor Author

@ruflin , yes I am referring to labels as well as tags.
I did the same, I have not documented neither labels nor tags.
Defining an empty object sounds better!

@ruflin
Copy link
Contributor

ruflin commented Feb 2, 2017

See #3515

@@ -0,0 +1,19 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs update but I can do that after merging.

@ruflin ruflin merged commit 15db49f into elastic:master Feb 3, 2017
@ruflin
Copy link
Contributor

ruflin commented Feb 3, 2017

@douaejeouit Thanks for the contribution.

@douaejeouit
Copy link
Contributor Author

With the greatest of pleasure @ruflin

@douaejeouit douaejeouit deleted the metricset-image branch February 3, 2017 11:00
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