Rework tasks to give more context about the check that was made. #74

Natim · 2017-08-28T15:45:53Z

Fixes #53
Fixes #58
Fixes #66

Natim · 2017-08-28T16:04:26Z

pollbot/tasks/archives.py

+                status = resp.status != 404
+                return {
+                    "status":  status and "exists" or "missing",
+                    "message": "Checking archive.mozilla.org release publication",


Exists
A repository with that release version exists in https://archive.mozilla.org/pub/firefox/releases/55.0/

Missing
No repository found for that version number: https://archive.mozilla.org/pub/firefox/releases/

Should the Exists message include a version number? (If so, probably "exists at" instead of "exists in".) Also, I don't know if "repository" is the right word -- maybe packages or archives.

Natim · 2017-08-28T16:06:31Z

pollbot/tasks/archives.py

+
+                return {
+                    "status":  status and "exists" or "missing",
+                    "message": "Checking archive.mozilla.org nightly publication",


Exists
The archive exists in nightly latest-date

Missing
The archive doesn't exists for this version in nightly latest-date

Exists
The archive exists in nightly latest-date-l10n

Missing
The archive doesn't exists for this version in nightly latest-date-l10n

The archive doesn't exist, or even No archive exists. I'd also like to add a noun to "nightly latest-date" because I still feel like my understanding of what it is is a little incomplete; maybe "nightly latest-date symlink" or "nightly latest-date mirror"?

Natim · 2017-08-28T16:07:48Z

pollbot/tasks/archives.py

+        else:
+            return {
+                "status": "missing",
+                "message": "No archive-date checks for {} releases".format(channel.value.lower()),


Differentiate archive-date and archive-date-l10n here.

Natim · 2017-08-28T16:08:49Z

pollbot/tasks/bedrock.py

+            status = resp.status != 404
+            return {
+                "status":  status and "exists" or "missing",
+                "message": "Checking bedrock for release note publication",


Exists
Release notes were found for that version.

Missing
No release notes were published for that version.

Error
We were unable to contact bedrock. (HTTP 503 - Service unavailable)

How about changing that version to version {version}?

Natim · 2017-08-28T16:09:35Z

pollbot/tasks/bedrock.py

-        return True
+        return {
+            "status": "missing",
+            "message": "No security advisories for {} releases".format(channel.value.lower()),


Missing
No security advisories publication for (beta|nightly) releases

Ah, this one tricked me for a second because I thought it meant they hadn't been published "yet", but actually they are not published as part of the process for these channels. It's correct as-is but maybe to make it stronger, how about Publication of security advisories does not happen for ... or Security advisories are never published for ...?

Natim · 2017-08-28T16:10:11Z

pollbot/tasks/bedrock.py

+            status = build_version_id(last_release) >= build_version_id(version)
+            return {
+                "status":  status and "exists" or "missing",
+                "message": "Checking bedrock for security advisories publication",


Exists
Securities advisories for release were published until version 55.0

Missing
Securities advisories for release were published until version 55.0

Error
We were unable to contact bedrock. (HTTP 503 - Service unavailable)

Security advisories. Also I'd probably change until into up to or through.

Natim · 2017-08-28T16:10:59Z

pollbot/tasks/bedrock.py

+            status = build_version_id(last_release) >= build_version_id(version)
+            return {
+                "status":  status and "exists" or "missing",
+                "message": "Checking bedrock for download links publication",


Exists
The download links for release have been published with version 55.0

Missing
The download links for release have been published with version 55.0

Error
We were unable to contact bedrock (HTTP 408 - Timeout)

Published with? I feel like this should be published for, but maybe I don't understand what the actual step of the release process is.

Natim · 2017-08-28T16:11:38Z

pollbot/tasks/product_details.py

+            status = '{}-{}'.format(product, version) in body['releases']
+            return {
+                "status":  status and "exists" or "missing",
+                "message": "Checking product-details for the release version",


Exists
We found product-details information about that version : {JSON STRING}

Missing
We did not found product-details information about that version

Error
We were unable to contact product-details (HTTP 503 - Service unavailable)

I'm pretty sure "that version" is correct but you might want to change it to version {version}. Also, no space before colon in English.

glasserc

I reviewed the messages in your comments, hope that's what I was supposed to do!

glasserc · 2017-08-28T16:16:39Z

API_CHANGELOG.rst

@@ -8,6 +8,9 @@ API Changelog
 - Add archive-date and archive-date-l10n checks for nightly
 - Add the ongoing-versions endpoint
 - Add the list of checks for a given version endpoint.
+- Add archive-date and archive-date-l10n nightly endpoints.


I think this is covered (above).

glasserc · 2017-08-28T16:22:05Z

pollbot/tasks/archives.py

+                status = resp.status != 404
+                return {
+                    "status":  status and "exists" or "missing",
+                    "message": "Checking archive.mozilla.org release publication",


Should the Exists message include a version number? (If so, probably "exists at" instead of "exists in".) Also, I don't know if "repository" is the right word -- maybe packages or archives.

glasserc · 2017-08-28T16:26:18Z

pollbot/tasks/archives.py

+
+                return {
+                    "status":  status and "exists" or "missing",
+                    "message": "Checking archive.mozilla.org nightly publication",


The archive doesn't exist, or even No archive exists. I'd also like to add a noun to "nightly latest-date" because I still feel like my understanding of what it is is a little incomplete; maybe "nightly latest-date symlink" or "nightly latest-date mirror"?

glasserc · 2017-08-28T16:27:01Z

pollbot/tasks/bedrock.py

+            status = resp.status != 404
+            return {
+                "status":  status and "exists" or "missing",
+                "message": "Checking bedrock for release note publication",


How about changing that version to version {version}?

glasserc · 2017-08-28T16:27:18Z

pollbot/tasks/bedrock.py

-        return True
+        return {
+            "status": "missing",
+            "message": "No security advisories for {} releases".format(channel.value.lower()),


Ah, this one tricked me for a second because I thought it meant they hadn't been published "yet", but actually they are not published as part of the process for these channels. It's correct as-is but maybe to make it stronger, how about Publication of security advisories does not happen for ... or Security advisories are never published for ...?

glasserc · 2017-08-28T16:30:16Z

pollbot/tasks/bedrock.py

+            status = build_version_id(last_release) >= build_version_id(version)
+            return {
+                "status":  status and "exists" or "missing",
+                "message": "Checking bedrock for security advisories publication",


Security advisories. Also I'd probably change until into up to or through.

glasserc · 2017-08-28T16:36:14Z

pollbot/tasks/bedrock.py

+            status = build_version_id(last_release) >= build_version_id(version)
+            return {
+                "status":  status and "exists" or "missing",
+                "message": "Checking bedrock for download links publication",


Published with? I feel like this should be published for, but maybe I don't understand what the actual step of the release process is.

glasserc · 2017-08-28T16:38:42Z

pollbot/tasks/product_details.py

+            status = '{}-{}'.format(product, version) in body['releases']
+            return {
+                "status":  status and "exists" or "missing",
+                "message": "Checking product-details for the release version",


I'm pretty sure "that version" is correct but you might want to change it to version {version}. Also, no space before colon in English.

leplatrem

If we centralize the task response in a function, we should use it everywhere.

Also, I have the feeling that having status as a boolean is very limiting. Will be hard to cover incomplete for example. Plus, the status strings are duplicated everywhere.
Maybe it's time to use a sort of Enum for status, and use it as much as possible.

leplatrem · 2017-08-30T09:11:48Z

pollbot/tasks/__init__.py

+            "link": link
+        }
+    else:
+        return {


nit: superflous else

leplatrem · 2017-08-30T09:12:37Z

pollbot/tasks/__init__.py

+            "status": "missing",
+            "message": missing_message,
+            "link": link
+        }


what about this way ?

return { "status": "exists" if status else "missing", "message": exists_message if status else missing_message, "link": link }

leplatrem · 2017-08-30T09:13:46Z

pollbot/tasks/__init__.py

@@ -23,3 +23,19 @@ def heartbeat_factory(url):
                    return True
                return False
    return heartbeat
+
+
+def build_task_response(status, exists_message, missing_message, link):


nit: exists vs. missing.. shouldn't it be existing ?

That's the name of the statuses.

leplatrem · 2017-08-30T09:15:17Z

pollbot/tasks/archives.py

+                    status = build_version_id(last_release) >= build_version_id(version)
+
+                exists_message = "The archive exists at {}".format(url)
+                missing_message = "No archive exists at {}".format(url)


nit: no archive found ?

leplatrem · 2017-08-30T09:16:36Z

pollbot/tasks/archives.py

+                "status": "missing",
+                "message": "No archive-date checks for {} releases".format(channel.value.lower()),
+                "link": url
+            }


Should use build_task_response() here, otherwise it's weird

Why? The build_task_response is to return a different message depending on a boolean, in that case it is always missing.

leplatrem · 2017-08-30T09:17:43Z

pollbot/tasks/bedrock.py

+            "message": "Security advisories are never published for {} releases".format(
+                channel.value.lower()),
+            "link": url
+        }


Should use build_task_reponse() here too

Let's discuss that it seems that we don't have the same vision of what build_task_response is for

leplatrem · 2017-08-30T09:18:37Z

pollbot/tasks/product_details.py

+            "status":  status and "exists" or "missing",
+            "message": "Checking product-details for the nightly version",
+            "link": 'https://product-details.mozilla.org/1.0/{}_versions.json'.format(product)
+        }


leplatrem · 2017-08-30T09:20:14Z

pollbot/tasks/product_details.py


    with get_session() as session:
        url = 'https://product-details.mozilla.org/1.0/{}.json'.format(product)
        async with session.get(url) as resp:
            if resp.status != 200:
-                msg = 'Product Details info not available  ({})'.format(resp.status)
+                msg = 'We were unable to contact product-details (HTTP {})'.format(resp.status)


"To contact" is not properly true here (the server responded something). The original error message was better.

leplatrem · 2017-08-30T09:56:32Z

pollbot/tasks/__init__.py

+        "status": Status.EXISTS.value if status else Status.MISSING.value,
+        "message": exists_message if status else missing_message,
+        "link": link
+    }


Why not use build_task_response(status, message, link) above then ?

leplatrem · 2017-08-30T09:57:20Z

pollbot/tasks/archives.py

-                "message": "No archive-date checks for {} releases".format(channel.value.lower()),
-                "link": url
-            }
+            return build_task_response(


nit: superflous else

leplatrem · 2017-08-30T10:00:51Z

pollbot/tasks/__init__.py

+    }
+
+
+def build_task_response_from_bool(status, exists_message, missing_message, link):


You could keep only one helper if you change the signature to:

def build_task_response(status, link, message, fail_message=None): if isinstance(status, bool): message = message if status else fail_message ...

Rework tasks to give more context about the check that was made.

cf93710

Natim requested a review from glasserc August 28, 2017 15:45

Update changelogs.

dc758e3

This was referenced Aug 28, 2017

One-line blurb about each category #63

Closed

Add messages for all status #53

Closed

Natim commented Aug 28, 2017

View reviewed changes

Natim added this to In Progress in Version 0.2.0 Aug 28, 2017

glasserc reviewed Aug 28, 2017

View reviewed changes

Rémy HUBSCHER added 2 commits August 29, 2017 18:12

Update messages.

7e88e74

@glasserc review.

cb0a65d

leplatrem requested changes Aug 30, 2017

View reviewed changes

@leplatrem review.

e7c9e6d

Natim force-pushed the 58-add-links-to-tasks branch from 4460297 to e7c9e6d Compare August 30, 2017 09:50

Natim requested a review from leplatrem August 30, 2017 09:52

leplatrem approved these changes Aug 30, 2017

View reviewed changes

@leplatrem review.

79181ff

Natim merged commit 543af2b into master Aug 30, 2017

Natim deleted the 58-add-links-to-tasks branch August 30, 2017 12:29

Natim moved this from In Progress to Complete in Version 0.2.0 Aug 30, 2017

		}


		def build_task_response_from_bool(status, exists_message, missing_message, link):

Rework tasks to give more context about the check that was made. #74

Rework tasks to give more context about the check that was made. #74

Conversation

Natim commented Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasserc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leplatrem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Natim commented Aug 28, 2017 •

edited

Loading