-
Notifications
You must be signed in to change notification settings - Fork 95
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
Bug 1901263: Add profileGroupId
to schemas.
#822
Conversation
I'm unsure what the failing test is about, I was seeing that locally too both with and without my change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the template, but misses rendered schemas. Can you also rebuild schemas following https://github.com/mozilla-services/mozilla-pipeline-schemas?tab=readme-ov-file#adding-a-new-schema?
Done, and added to the validations. I'm completely unable to run tests for some reason though so I'm not sure if they are passing. |
Thank you. Integration tests do not run for forked PRs, I'm triggering them manually here. There was an issue with dependencies causing tests to fail that I just fixed in #823. Can you rebase this PR on the latest |
This adds the `profileGroupId` added to the client side in bug 1901263 to the schemas wherever the `clientId` is present. I believe that every ping including `clientId` will include `profileGroupId` however since the new property is not marked as required in any schema (as it cannot be to avoid breaking older versions of Firefox) if new pings are introduced that do not include the property then it should not break anything.
Ok rebasing and switching to Linux allowed me to run the tests locally and they passed |
Thank you, this is ready to be merged now. @chutten just to double check since you seem to have been involved in the implementation - is this schema change good to be merged and land in BQ now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chutten just to double check since you seem to have been involved in the implementation - is this schema change good to be merged and land in BQ now?
Adding an optional field sounds safe, and the client-side changes are lined up and ready to land as this does. So from my POV it seems good to merge.
Thanks, how long does it take for this to go to prod, or rather when can I land the client side pieces? |
This change should land in BigQuery tomorrow. |
This adds the
profileGroupId
being added to the client side in bug 1901263 to the schemas wherever theclientId
is present. I believe that every ping includingclientId
will includeprofileGroupId
however since the new property is not marked as required in any schema (as it cannot be to avoid breaking older versions of Firefox) if new pings are introduced that do not include the property then it should not break anything.Checklist for reviewer:
./.github/push-to-trigger-integration <username>:<branchname>
For glean changes:
templates/include/glean/CHANGELOG.md
For modifications to schemas in restricted namespaces (see
CODEOWNERS
):