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

Fixes issue #62 - Changed agentHost field name to match CEF docs #75

Merged
merged 3 commits into from
Dec 20, 2019

Conversation

n3g
Copy link
Contributor

@n3g n3g commented Nov 15, 2019

This commit fixes issue #62.
According to CEF documentation, the short name 'ahost' corresponds to 'agentHostName' and not 'agentHost' as it's being used now.

Note: this might have impact on code already using the incorrect mapping.

This commit fixes issue logstash-plugins#62.
According to CEF documentation, the short name 'ahost' corresponds to 'agentHostName' and not 'agentHost' as it's being used now.
@kares
Copy link
Contributor

kares commented Dec 17, 2019

Thanks Ricardo, this seems correct.
Checked the old v23 documentation, current is v25 and they both use agentHostName

@kares
Copy link
Contributor

kares commented Dec 17, 2019

There should have been a failing spec with this change ... around
https://github.com/logstash-plugins/logstash-codec-cef/blob/master/spec/codecs/cef_spec.rb#L219
and
https://github.com/logstash-plugins/logstash-codec-cef/blob/master/spec/codecs/cef_spec.rb#L230
... since its testing with the old mapping.

CI did not run here so we're not seeing the 🔴 here, could you please look at those?

@kares
Copy link
Contributor

kares commented Dec 17, 2019

@elasticcla
Copy link

Hi @n3g, 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 your 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?

1 similar comment
@elasticcla
Copy link

Hi @n3g, 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 your 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?

@n3g
Copy link
Contributor Author

n3g commented Dec 18, 2019

Hi @kares ,
Thanks for the warning, I didn't notice anything wrong as everything was green, but I think I've made the necessary changes in this last commit.

@elasticsearch-bot elasticsearch-bot self-assigned this Dec 18, 2019
@colinsurprenant
Copy link
Contributor

  • This is somewhat of a duplicate for Update cef.rb #45 which also includes other field fixes. I am ok in moving forward with this fix though.
  • I am unsure about version bump; looking at the changelog it seems we've allowed patch level bump for such a field name changes but to me this seems like a breaking change. Thoughts?

@kares
Copy link
Contributor

kares commented Dec 18, 2019

did not see #45 ... should review the field updates, later. in terms of version lets do 6.1 (definitely not patch but inc minor)

@colinsurprenant
Copy link
Contributor

@n3g would you be able to add a version bump + changelog commit?

@n3g
Copy link
Contributor Author

n3g commented Dec 19, 2019

@kares, @colinsurprenant: should I bump to 6.1.0 then? Current one is 6.0.1.

@colinsurprenant
Copy link
Contributor

@n3g yes, bump to 6.1.0 and add to the CHANGELOG. Thanks!

@n3g
Copy link
Contributor Author

n3g commented Dec 20, 2019

There seems to be an error in the tests, but it's not related to the change. Any idea how to rerun this?

@colinsurprenant
Copy link
Contributor

colinsurprenant commented Dec 20, 2019

@n3g restarted the build. LGTM pending green.

@colinsurprenant colinsurprenant merged commit b8d8dcd into logstash-plugins:master Dec 20, 2019
@colinsurprenant
Copy link
Contributor

v6.1.0 published!
Thanks for your contribution @n3g !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants