-
Notifications
You must be signed in to change notification settings - Fork 535
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
ADI export script (bug 1014752) #115
ADI export script (bug 1014752) #115
Conversation
def handle(self, *args, **options): | ||
start = datetime.now() # Measure the time it takes to run the script. | ||
day = options['date'] | ||
if day is None: |
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.
Can't we default to the date we will be using (I guess "yesterday")?
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.
I prefer to be explicit on that one given the amount of time/resource it will consume.
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.
Fair enough :)
General note: we should consider doing a PR to pysh2 for the yieldcursor thing. It's worth it in IMHO as we are now adding it as a AMO dependency. |
The PR for pyhs2 already exists: BradRuderman/pyhs2#17 |
raise CommandError('Please specify at least one of --with-updates ' | ||
'or --with-downloads.') | ||
|
||
with ClevererConnection(host='peach-gw.peach.metrics.scl3.mozilla.com', |
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.
no need to obfuscate those credentials with a os.environ.get() or something like that?
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.
Those are only accessible through the vpn, so I believe this is ok, please let us know if it isn't the case @jasonthomas
app_os, locale, update_type = splitted | ||
try: | ||
counter = int(counter) | ||
except ValueError: # Badly formatted? Drop. |
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.
Maybe we wanna log this king of skip? In case we have some counter weirdness once in prod, this may help.
ADI export script (bug 1014752)
Fixes bug 1014752