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

Added device model and brand to user_agent output #77

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fabiogermann
Copy link

Java tests are passing - the JRuby ones - I have some question.

@@ -27,7 +28,44 @@
*/
final class Device {

public static String fromMap(Map<String, String> m) {
return m.get("family");
public final String family;
Copy link
Contributor

Choose a reason for hiding this comment

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

seems that the uap-java does not have these changes:
https://github.com/ua-parser/uap-java/blob/master/src/main/java/ua_parser/Device.java

so this was written from scratch or copied from somewhere else?

Copy link
Author

Choose a reason for hiding this comment

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

@kares this was written from scratch (similar to the OS class). I did not find a reference in the java class that it was from the uap-java project.

Copy link
Author

Choose a reason for hiding this comment

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

Now that you pointed me to it - there is a PR ua-parser/uap-java#37 in the uap-java project that asks for same functionality but it hasn't seen action since early 2020. If I had seen it, I would have mentioned it.

fail-fast: false
matrix:
os: [ubuntu-20.04]
ruby: [jruby-9.1.17.0]
Copy link
Contributor

@kares kares Dec 20, 2021

Choose a reason for hiding this comment

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

we do not want this file, currently the plugin tests against Travis CI, for whatever reason the CI did not kick-off
maybe try removing the file - build tried running against 55c2d94 but somehow CI mis-detected the commit:

ua

Copy link
Author

Choose a reason for hiding this comment

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

I deleted the file and pushed again

@device_family_field = "#{target}#{@device_family_field}"
@device_brand_field = ecs_select[disabled: "[#{@prefix}device]", v1: '[device][brand]']
@device_brand_field = "#{target}#{@device_brand_field}"
@device_model_field = ecs_select[disabled: "[#{@prefix}device]", v1: '[device][model]']
Copy link
Contributor

Choose a reason for hiding this comment

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

these are going to be problematic as ATM ECS (1.11) only supports user_agent.device.name

Copy link
Author

Choose a reason for hiding this comment

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

@kares is there an "experimental" switch that could enable this behavior? Such that it is still compliant?

@fabiogermann
Copy link
Author

@kares how do we want to proceed here?

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

Successfully merging this pull request may close these issues.

4 participants