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.

Sign up for GitHub

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

Jump to bottom

1624695: Act on changes in upload enable state outside of application #791

Merged
merged 2 commits into from
Apr 2, 2020
Merged

1624695: Act on changes in upload enable state outside of application #791

merged 2 commits into from
Apr 2, 2020

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Mar 30, 2020 •
edited
Loading

I found a much simpler way forward here that really only adds some slightly different logic in Glean::new and a bit of a refactor. I'm much happier with this than the earlier two approaches.

TODO:

  • Port new unit tests to Swift (I don't currently have access to a Mac -- volunteers?)

@auto-assign auto-assign bot requested a review from Dexterp37 March 30, 2020 22:54
@mdboom mdboom requested review from badboy and removed request for Dexterp37 March 30, 2020 22:55
This was referenced Mar 30, 2020
Closed
Closed
badboy
badboy requested changes Mar 31, 2020
View reviewed changes
.dictionary
@@ -79,6 +79,7 @@ codecov
codepoints
commandline
conda
config
Copy link
Member

Choose a reason for hiding this comment

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

Wonder if we maybe should just use the proper form "configuration" in text then.
(but I see it's "config file" down below, which seems fine to me)

CHANGELOG.md Outdated
@@ -13,6 +13,7 @@
* `glean_parser` now makes it easier to write external translation functions for
different language targets.
* BUGFIX: glean_parser now works on 32-bit Windows.
* Glean will now detect when the upload enabled flag changes outside of the application, for example due to a change in a config file. This means that if upload is disabled while the application wasn't running (e.g. between the runs of a Python command using the Glean SDK), the database is correctly cleared and a deletion request ping is sent.
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a link to the bug here (or to the PR as we do for other items in the changelog)?

glean-core/src/lib.rs Outdated Show resolved Hide resolved
glean-core/src/lib.rs Outdated Show resolved Hide resolved
glean-core/src/lib.rs Outdated Show resolved Hide resolved
glean-core/src/lib.rs Outdated Show resolved Hide resolved
@mdboom mdboom requested a review from badboy March 31, 2020 12:23
@mdboom mdboom force-pushed the deletion-request-at-init-simple branch from 46fd2f0 to 91e2041 Compare March 31, 2020 12:23
@mdboom
Copy link
Contributor Author

mdboom commented Mar 31, 2020

This includes a bug fix for the metrics ping scheduler sending on the last day of the month. Does anyone feel strongly about separating that out?

@mdboom mdboom force-pushed the deletion-request-at-init-simple branch from 65ef8a6 to 1b6bfe5 Compare March 31, 2020 14:47
@badboy
Copy link
Member

badboy commented Apr 1, 2020

This includes a bug fix for the metrics ping scheduler sending on the last day of the month. Does anyone feel strongly about separating that out?

As long as it is its own commit, I'm fine.

badboy
badboy approved these changes Apr 1, 2020
View reviewed changes
Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

r+wc.

I'd prefer to clean up the commits for a nice history and separate out the iOS bugfix while you're at it.
minimal nits left, but otherwise good to go.

glean-core/ios/GleanTests/Scheduler/MetricsPingSchedulerTests.swift
@@ -125,7 +125,7 @@ class MetricsPingSchedulerTests: XCTestCase {
byAdding: .day,
value: 1,
to: now,
wrappingComponents: true
wrappingComponents: false
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this to its own commit and then give it a proper commit message?

Copy link
Member

Choose a reason for hiding this comment

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

while you're at it you can then squash the remaining commits into a single useful one.

glean-core/ios/Glean/Scheduler/MetricsPingScheduler.swift
@@ -59,7 +59,7 @@ class MetricsPingScheduler {
byAdding: .day,
value: 1,
to: fireDate,
wrappingComponents: true
wrappingComponents: false
Copy link
Member

Choose a reason for hiding this comment

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

That should go into the separated-out commit.

glean-core/src/lib.rs Outdated Show resolved Hide resolved
@mdboom mdboom force-pushed the deletion-request-at-init-simple branch from b5f7ee5 to b41fcf1 Compare April 1, 2020 12:30
@mdboom mdboom merged commit c3b0d64 into mozilla:master Apr 2, 2020
@mdboom mdboom deleted the deletion-request-at-init-simple branch April 14, 2020 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@badboy badboy badboy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mdboom @badboy @travis79