Initial code commit #1

gregeinfrank · 2019-11-15T02:33:21Z

This is a WIP PR while I work through tests and TODOs

gohelankit · 2020-02-11T02:17:04Z

src/asana/helpers.py

+
+
+def _task_name_from_pull_request(pull_request: PullRequest) -> str:
+    return "#{} - {}".format(pull_request.number(), pull_request.title())


Can we use f-strings everywhere we need to do string formatting?

If we use temporary variables instead of expressions in f-strings, then I would agree. Embedding code in f-string {} blocks is generally bad for readability/maintainability, IMO :)

e.g.

def _task_name_from_pull_request(pull_request: PullRequest) -> str: pr_number = pull_request.number() pr_title = pull_request.title() return f"#{pr_number} - {pr_title}"

rather than

def _task_name_from_pull_request(pull_request: PullRequest) -> str: return f"#{pull_request.number()} - {pull_request.title()}"

Yeah I think I agree with this ^^ and that it's not worth splitting into 3 lines for that reason

gohelankit · 2020-02-11T02:20:44Z

src/asana/helpers.py

+    return _wrap_in_tag("body")(
+        _wrap_in_tag("em")(
+            "This is a one-way sync from GitHub to Asana. Do not edit this task or comment on it!"
+        )
+        + f"\n\n\uD83D\uDD17 {url}"
+        + "\n✍️ "
+        + asana_author_url
+        + _wrap_in_tag("strong")("\n\nDescription:\n")
+        + escape(pull_request.body())
+    )


Somewhere in here we would need the task status text right? Task is closed/not closed because...

Yeah, maybe I'll just add a task for tracking that - I took it out explicitly because it just seemed like noise but maybe it's useful for some people

gohelankit · 2020-02-11T02:51:23Z

src/github/graphql/client.py

@@ -0,0 +1,47 @@
+from typing import Tuple
+from sgqlc.endpoint.http import HTTPEndpoint


I was looking into this GraphQL client - and I think we should be able to do something along the lines of this to ensure that we can do away with our schema file, and be able to write this

query { repository(owner: "profusion", name: "sgqlc") { issues(first: 100) { nodes { number title } } } }

as

op = Operation(Query) issues = op.repository(owner=owner, name=name).issues(first=100)

They have an example of how to use their code generator that can output a schema file which we can directly query against in python code. (We would merge this in its current form and then I could work on changing how we use GraphQL)

gohelankit · 2020-02-11T02:57:58Z

src/github/models/pull_request.py

+        if len(maybe_multi_assignees) == 1:
+            return maybe_multi_assignees[0]
+        elif len(maybe_multi_assignees) == 0:
+            logger.info("GitHub PR has no assignees. Choosing author as assignee")


This text is also currently something that is part of the task description

I didn't feel the need - but can add a task for it

gohelankit · 2020-02-11T03:02:24Z

src/github/models/pull_request.py

+            logger.info(
+                "GitHub PR has multiple assignees: {} Choosing first one in alphabetical order as as assignee: {}".format(
+                    maybe_multi_assignees, assignee
+                )
+            )


Same with this text - we would need some way to make sure the _task_description_from_pull_request method gets this message - the current Clojure code returns an object of the form

{ assignee: ... warning: ... }

and displays these warnings if present

gohelankit · 2020-02-11T03:03:18Z

src/github/utils.py

@@ -0,0 +1,7 @@
+from datetime import datetime


This file isn't related to github, so we could move it to the top level directory.

rohansub-asana · 2020-02-11T07:01:42Z

src/asana/client.py

+    client.tasks.add_followers(task_id, {"followers": followers})
+
+
+def add_comment(task_id: str, comment_body: str):


nit: str return type

rohansub-asana · 2020-02-11T07:22:18Z

src/asana/helpers.py

+        "completed": _task_completion_from_pull_request(pull_request),
+        "followers": _task_followers_from_pull_request(pull_request),
+    }
+


I'm open to working on splitting out some of the functions in this file into a couple different files (one for creating asana comment from git comment, another for task from pr, etc.) and doing some general cleanup! I feel like not all of this logic should be just in a helpers file.

padresmurfa

LGTM!

I believe there might be a major piece of functionality missing, i.e. adding newly created SGTM tasks to a pre-configured / pre-created project. This should probably be addressed prior to using SGTM in Asana, but does not need to be addressed in this PR. I added an Asana task for this.

None of my other review comments need immediate action, and I am happy to do them all myself in subsequent PRs, assuming that you agree with them.

padresmurfa · 2020-02-11T08:07:49Z

terraform/main.tf

+provider "aws" {
+  region = "us-east-1"
+  shared_credentials_file = "/Users/gregeinfrank/.aws/credentials"
+  profile = "sgtm"
+}


I believe that this information should be specified via Terraform variables, rather than hardcoded in the script

Yeah - this was for my testing because I wanted to override the profile to sgtm (pointing to my personal AWS account) -- I'll leave out of this PR

padresmurfa · 2020-02-11T08:10:12Z

terraform/main.tf

+  # The filebase64sha256() function is available in Terraform 0.11.12 and later
+  # For Terraform 0.11.11 and earlier, use the base64sha256() function and the file() function:
+  # source_code_hash = "${base64sha256(file("lambda_function_payload.zip"))}"
+  source_code_hash = "${filebase64sha256("../function.zip")}"


I feel that this should not require the user to edit the source code to achieve the correct result. A few possible solutions to this could include:

only supporting Terraform 0.11.12 and later

using the interpolation syntax, possibly along with templates, to achieve the desired version specific results.

providing different versions of the .tf files for different versions of Terraform, and using a shell script to determine the Terraform version and use the appropriate files

using the approach we use in Asana with .yaml.py files, e.g. main.tf.py is a python script that outputs main.tf, using whatever means it finds appropriate to create the file, such as using the Python template system, or python string-replace on well-known-strings. A runner script of some sort would handle this transformation phase.

For example:

`
template = """
...
handler = "src.handler.handler"
source_code_hash = "@@SOURCE_CODE_HASH@@"
...
"""

if terraform_11_12_or_later:
output = template.replace("@@SOURCE_CODE_HASH@@", "${filebase64sha256("../function.zip")}")
else:
output = template.replace("@@SOURCE_CODE_HASH@@", """${base64sha256(file("lambda_function_payload.zip"))}""")

print(output)
`

padresmurfa · 2020-02-11T08:14:17Z

terraform/main.tf

+}
+
+resource "aws_lambda_function" "sgtm" {
+  filename      = "../function.zip"


I'm assume at this point that I will see later in the review that this zip file is the output of some command executed on the command line. This command should be documented somewhere, e.g. in a README.md file. An alternative would be to use a scripts directory, as mentioned elsewhere in this review.

Actually, I don't see anything in the project that makes it obvious what the functions.zip file should contain precisely, and how it should be created. I'd guess that zipping the src directory, with path info included, would do the trick, but this should be documented.

Yeah - on the TODO list for deployment strategy. I've done this manually so far in testing but will add a script and deployment instructions

padresmurfa · 2020-02-11T08:41:55Z

terraform/main.tf

+
+  runtime = "python3.7"
+
+  timeout = 30


I believe that this should be a Terraform variable

padresmurfa · 2020-02-11T08:44:34Z

terraform/main.tf

+  }
+}
+
+resource "aws_api_gateway_resource" "sgtm_resource" {


A single-line(or so) comment for each resource, explaining what it is in plain english, would be great for readability.

Cool - i'll add a task for that

padresmurfa · 2020-02-11T11:29:46Z

src/github/logic.py

+    reviews = [
+        review
+        for review in pull_request.reviews()
+        if review.is_approval_or_changes_requested()
+        and review.submitted_at() < merged_at
+    ]
+    if reviews:
+        latest_review = sorted(reviews, key=lambda r: r.submitted_at())[-1]
+        return latest_review.is_approval()


I feel that reviews would be better named premerge_reviews for clarity.

padresmurfa · 2020-02-11T11:37:48Z

src/github/logic.py

+    comments = [
+        comment
+        for comment in pull_request.comments()
+        if comment.published_at() > merged_at
+    ]
+    reviews = [


likewise, I'd suggest using the terms postmerge_comments and postmerge_reviews here for increased clarity.

padresmurfa · 2020-02-11T11:47:42Z

test/github/helpers.py

@@ -0,0 +1,132 @@
+from uuid import uuid4


it seems to me from the contents of this file that it could be named request_builders.py, which would be an improvement over the generic "helpers" name.

Hm request_builders doesn't seem right because I don't think it's really building "requests" - how about github/stub_builders.py?

padresmurfa · 2020-02-11T11:52:02Z

test/mock_test_case.py

@@ -0,0 +1,45 @@
+"""


this file should probably be renamed something like mock_dynamobdb_test_case

padresmurfa · 2020-02-11T11:52:52Z

test/mock_test_case.py

+
+
+@mock_dynamodb2
+class MockTestCase(TestCase):


and likewise, the test case should be named something like MockDynamoDbTestCase, as the name implies that this is more generic than it actually is.

Yeah I think my original plan was to build on this and add more default mocks, but since that didn't happen - that sounds reasonable

gregeinfrank · 2020-02-11T16:27:09Z

Thanks for the comments - I tried to address everything or open a task for it. I'm going to merge so people can start opening up branches

gregeinfrank added 3 commits November 14, 2019 18:32

Initial code commit

6ffe1e5

More tests

bcdae58

Add readme

3bacd90

gohelankit reviewed Feb 11, 2020

View reviewed changes

rohansub-asana reviewed Feb 11, 2020

View reviewed changes

padresmurfa approved these changes Feb 11, 2020

View reviewed changes

padresmurfa assigned gregeinfrank Feb 11, 2020

gregeinfrank added 2 commits February 11, 2020 08:13

Rename github.utils to utils

125934f

PR comments

c579a53

gregeinfrank merged commit cf7024b into master Feb 11, 2020

gregeinfrank deleted the gregeinfrank-initital-commit branch February 11, 2020 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial code commit #1

Initial code commit #1

gregeinfrank commented Nov 15, 2019

gohelankit Feb 11, 2020

padresmurfa Feb 11, 2020

gregeinfrank Feb 11, 2020

gohelankit Feb 11, 2020

gregeinfrank Feb 11, 2020

gohelankit Feb 11, 2020

gohelankit Feb 11, 2020

gregeinfrank Feb 11, 2020

gohelankit Feb 11, 2020

gohelankit Feb 11, 2020

rohansub-asana Feb 11, 2020

rohansub-asana Feb 11, 2020

padresmurfa left a comment

padresmurfa Feb 11, 2020

gregeinfrank Feb 11, 2020

padresmurfa Feb 11, 2020

padresmurfa Feb 11, 2020

padresmurfa Feb 11, 2020

gregeinfrank Feb 11, 2020

padresmurfa Feb 11, 2020

gregeinfrank Feb 11, 2020

padresmurfa Feb 11, 2020

gregeinfrank Feb 11, 2020

padresmurfa Feb 11, 2020

padresmurfa Feb 11, 2020

padresmurfa Feb 11, 2020

gregeinfrank Feb 11, 2020

padresmurfa Feb 11, 2020

padresmurfa Feb 11, 2020

gregeinfrank Feb 11, 2020

gregeinfrank commented Feb 11, 2020



		def _task_name_from_pull_request(pull_request: PullRequest) -> str:
		return "#{} - {}".format(pull_request.number(), pull_request.title())

		@@ -0,0 +1,47 @@
		from typing import Tuple
		from sgqlc.endpoint.http import HTTPEndpoint

		client.tasks.add_followers(task_id, {"followers": followers})


		def add_comment(task_id: str, comment_body: str):



		@mock_dynamodb2
		class MockTestCase(TestCase):

Initial code commit #1

Initial code commit #1

Conversation

gregeinfrank commented Nov 15, 2019

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

padresmurfa 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregeinfrank commented Feb 11, 2020