-
Notifications
You must be signed in to change notification settings - Fork 222
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
Fix Bug 909930 ensure that crash_trends does not cause 500 errors when no Nightly #1498
Fix Bug 909930 ensure that crash_trends does not cause 500 errors when no Nightly #1498
Conversation
if release['product'] == product: | ||
# First, line up all of the Nightlies | ||
if release['release'].lower() == 'nightly': | ||
versions.append(release['version']) |
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 you change that to
if release['release'].lower() == 'nightly':
version = release['version']
break
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.
It does the same as the next 5 lines below.
Ping me when jenkins passes on this. |
Try this inside your branch if your hooks aren't working.
|
# We did not find a featured Nightly, let's then simply use the latest | ||
for release in context['currentversions']: | ||
if release['product'] == product: | ||
# First, line up all of the Nightlies |
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 comment doesn't make any sense to me.
"line up"??
It's so incredibly clear what the if statements do that they don't actually need any comments.
What I am a bit confused about is what's going on on line 1804. There you accept either Nightly
or Aurora
but here you only accept Nightly
. Why is it different?
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.
So, I discovered this inconsistency whilst working on this bug and discussed it with Kaira, but it seems the person that needs to make the call is chofmann and I am still awaiting his feedback. With that, I did not want to just go ahead and change this now but, it should not prevent the PR from being merged and the current bug closed.
Why do we even need case insensitive comparisons? Where is it spelled both |
@peterbe see https://bugzilla.mozilla.org/show_bug.cgi?id=915162 - There are instances in the database where the release name is Nightly and others where it is nightly, the same for Aurora, Beta, Release etc. |
Either way, we should make the setting "Nightly" and "Aurora" and then in the code we do case insensitive comparisons. |
r+ |
…oducts-return-500-err Fix Bug 909930 ensure that crash_trends does not cause 500 errors when no Nightly
@peterbe r?
The best way to test this, besides running the tests ;), would be to test against the products that was returning the 500 errors mention in bug 909930.
This fixes the following bugs:
https://bugzilla.mozilla.org/show_bug.cgi?id=909930
https://bugzilla.mozilla.org/show_bug.cgi?id=887536
Implements the work around for:
https://bugzilla.mozilla.org/show_bug.cgi?id=915162
And fixes point 1 of:
https://bugzilla.mozilla.org/show_bug.cgi?id=914933