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

Vsphere module: collect custom fields from virtual machines (#4464) #4469

Merged
merged 2 commits into from
Jul 17, 2017
Merged

Vsphere module: collect custom fields from virtual machines (#4464) #4469

merged 2 commits into from
Jul 17, 2017

Conversation

amandahla
Copy link

To obtain custom fields values.
The test is missing this part because simulator doesn't have custom field manager.

@amandahla
Copy link
Author

I think I made a little mess using rebase...My commit to fix used memory field is here too. If it's a problem, I can try to remove him. :-|

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically on build-eu-00. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@exekias
Copy link
Contributor

exekias commented Jun 9, 2017

jenkins test it please

@exekias exekias assigned acesir and unassigned acesir Jun 9, 2017
@exekias
Copy link
Contributor

exekias commented Jun 9, 2017

@amandahla you can always try to remove it using git rebase -i HEAD~3, but you have to be careful, anyway it doesn't matter if the other one goes in

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.

PR is looking good, I have some questions:

  • What do custom fields look like normally? I'm worried about mapping explosion
  • Would it make sense to let the user decide if she wants them or not?

// Get all custom fields.
customFieldsOk := true

fieldMap := make(map[int32]string)
Copy link
Contributor

Choose a reason for hiding this comment

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

This name can be misleading, perhaps customFieldsMap is more clear? what do you think?

@amandahla
Copy link
Author

@exekias Good point. I didn't think about it. I'll make the changes (including the name of the field to customFieldsMap).
I didn't found in the docs if there is a limit so it would be nice/safer to leave this optional.

@tsg tsg added the review label Jun 9, 2017
@exekias
Copy link
Contributor

exekias commented Jun 12, 2017

LGTM, jenkins test it

@exekias
Copy link
Contributor

exekias commented Jun 13, 2017

@amandahla it seems test are failing because of an issue fixed in master, could you please rebase master so it can pass tests?

@amandahla
Copy link
Author

@exekias Is something that I'm missing here? :-(
I compare the two and didn't found differences
master...estaleiro:iss4464

@exekias
Copy link
Contributor

exekias commented Jun 14, 2017

jenkins test this please

@monicasarbu monicasarbu added the Metricbeat Metricbeat label Jun 26, 2017
@monicasarbu
Copy link
Contributor

@amandahla Can you please rebase to master?

@amandahla
Copy link
Author

amandahla commented Jun 29, 2017

There is a error on building filebeat. Is something that I did wrong? The branch has no conflicts.

@exekias
Copy link
Contributor

exekias commented Jul 11, 2017

Hi @amandahla, now that #4462 is finally merged, perhaps you can rebase this PR? I think the error will go away

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.

6 participants