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

Update/mk #159

Merged
merged 17 commits into from
Nov 9, 2018
Merged

Update/mk #159

merged 17 commits into from
Nov 9, 2018

Conversation

lorenzoPrimi
Copy link
Contributor

No description provided.

Copy link
Member

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

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

LGTM with a few comments 🐳

software_name = "ooniprobe-android";
software_version = VersionUtils.get_software_version();
randomize_input = 0;
randomize_input = false;
Copy link
Member

Choose a reason for hiding this comment

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

So, no-one is using randomize_input. I'd like to get rid of it in MK. It sounds like extra complexity now.

client.setNetworkType(network_type);
client.setPlatform("android");
//TODO what to add here?
//client.setProbeFamily(String value);
Copy link
Member

Choose a reason for hiding this comment

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

We do not use this field presently.

client.setPlatform("android");
//TODO what to add here?
//client.setProbeFamily(String value);
client.setProbeTimezone(TimeZone.getDefault().getDisplayName(true, TimeZone.SHORT));
Copy link
Member

Choose a reason for hiding this comment

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

What string would that be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be "UTC" or "CEST" or the short version that doesn't include the Country/City

Copy link
Member

Choose a reason for hiding this comment

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

Unless this was discussed and I missed the discussion, I would suggest to include the Country/City because it is information useful to us. Isn't it?

client.setSoftwareName(software_name);
client.setSoftwareVersion(software_version);
//TODO add @"web_connectivity", @"whatsapp", @"telegram", @"facebook_messenger", @"ndt", @"dash", @"http_invalid_request_line", @"http_header_field_manipulation"
//client.addSupportedTest();
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I suggest doing that :-)

@lorenzoPrimi lorenzoPrimi merged commit 30ed586 into release/2.0.0 Nov 9, 2018
@lorenzoPrimi lorenzoPrimi deleted the update/mk branch November 9, 2018 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants