Skip to content

Fix 1504200: Add ping validation feature to glean_parser#15

Merged
mdboom merged 1 commit into
mozilla:masterfrom
mdboom:validate-pings
Nov 21, 2018
Merged

Fix 1504200: Add ping validation feature to glean_parser#15
mdboom merged 1 commit into
mozilla:masterfrom
mdboom:validate-pings

Conversation

@mdboom
Copy link
Copy Markdown
Contributor

@mdboom mdboom commented Nov 19, 2018

This adds a feature to glean_parser to validate pings. It's basically a straightforward use of the jsonschema library that the backend uses to do that.

I've just copied the ping schema into this repo rather than doing anything fancy like fetching it from a URL because:

  1. We don't want to fetch from the URL every time we run a unit test in Glean. We could cache it, but cache invalidation is hard 😢
  2. We're already relying on changes to the ping schema that aren't yet merged into mozilla-pipeline-schemas. At least until things settle down we probably want to be more nimble and have the ability to put what we need to in here.

We definitely can and should revisit that later.

Copy link
Copy Markdown
Contributor

@Dexterp37 Dexterp37 left a comment

Choose a reason for hiding this comment

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

The idea looks great, thanks for tackling this. I think that we should not have multiple source of truth, wrt the schema. Let's have it only in one place. If it's already out of sync, let's update the main schema.

@@ -0,0 +1,424 @@
{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think I have strong feelings about this, given that I messed up quite a few time Desktop Telemetry due to out of sync dependencies (that I forgot to update :) ).

What about we keep this generic, drop the schema off this repo, pass in a path to the schema when invoking the command?



def test_validate_ping():
content = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we're moving the schema off this repo, let's keep this shorter: let's use a small JSON and provide a sample schema stub.

Comment thread glean_parser/validate_ping.py Outdated
Copy link
Copy Markdown
Contributor

@Dexterp37 Dexterp37 left a comment

Choose a reason for hiding this comment

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

The idea looks great, thanks for tackling this. I think that we should not have multiple source of truth, wrt the schema. Let's have it only in one place. If it's already out of sync, let's update the main schema.

@mdboom
Copy link
Copy Markdown
Contributor Author

mdboom commented Nov 20, 2018

With respect to managing the schema:

  • Since this is for unit testing, we shouldn't just link to the current HEAD, since all historical versions of glean wouldn't be expected to always work with the latest version of the schema, and this will make things like git blame much harder. At a minimum we should link to a particular git revision of the schema. Down the road when things settle down, we could link to a particular version number of the schema, but the overhead of managing version numbers in mozilla-pipeline-schemas is too high for the current phase of the project.

  • If we link to a specific revision, managing the cache should also be much easier.

@mdboom
Copy link
Copy Markdown
Contributor Author

mdboom commented Nov 20, 2018

I've updated this to allow the end user to specify the schema URL. It is cached, unless it's pointing to master which is a moving target and probably shouldn't be cached.

Thanks for another round of review, @Dexterp37

@mdboom mdboom force-pushed the validate-pings branch 2 times, most recently from 7be1ec8 to 1b35fb2 Compare November 20, 2018 23:07
Copy link
Copy Markdown
Contributor

@Dexterp37 Dexterp37 left a comment

Choose a reason for hiding this comment

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

Nice job!

@mdboom mdboom merged commit 0a6dea9 into mozilla:master Nov 21, 2018
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.

2 participants