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

Add the file formats to the README.md file #27

Merged
merged 1 commit into from
Jan 18, 2018

Conversation

Dexterp37
Copy link
Contributor

This additionally removes the empty README file. This should land, ideally, after #25 gets merged.

@Dexterp37 Dexterp37 self-assigned this Jan 16, 2018
@Dexterp37 Dexterp37 requested a review from georgf January 16, 2018 10:59
Copy link
Contributor

@georgf georgf left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good.
As consumers / interested parties, let's have @sunahsuh & @fbertsch have a look.

@sunahsuh
Copy link

Awesome, seems pretty clear to me :) I have a few inline comments.

"history": {
"<channel>": [
{
"cpp_guard": <string or null>,

Choose a reason for hiding this comment

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

Could we get a description of this field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! This probe data comes directly from the Telemetry probe definition. I've added a note at the bottom of this section with a link to the Telemetry docs with the full documentation on all the fields that can be found there.

"optout": true,
"revisions": {
"first": "320642944e42a889db13c6c55b404e32319d4de6",
"last": "6f5fac320fcb6625603fa8a744ffa8523f8b3d71"

Choose a reason for hiding this comment

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

Now that I see an example entry, I'm a bit concerned about ease of getting "probes active in the current latest release in each channel", which is a pretty common pattern

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The probe dictionary does this by getting all the probes for all the channels and then filtering for revision.first == <current revision>. I think we can probably make it better though: do you want to file another issue on this repo?

This additionally removes the empty README file.
@Dexterp37
Copy link
Contributor Author

Let me go ahead and merge this, since this is just a doc update. Feel free to open another issue about changing the API if needed, @sunahsuh !

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