[clean-repository-handling] Clean repository handling by wlonk · Pull Request #27 · oddbird/Metecho

wlonk · 2019-10-09T21:41:37Z

See https://trello.com/c/fsDdnJmD/101-chore-clean-up-repository-url-handling-in-admin

wlonk · 2019-10-14T19:31:31Z

@davisagli on the old PR, you mentioned:

@wlonk @jgerigmeyer for some other projects we've used the github api to look up github's integer id for the repo. that will be consistent even if the repo gets renamed or transferred to a different owner.
I think that's a good approach, and will implement it here.

wlonk · 2019-10-14T21:12:36Z

@davisagli ready for you to look at that change if you want.

* master:

jgerigmeyer

@wlonk Part of the purpose of this is to use IDs as the unique identifier when comparing repos. So I would expect it to be used in e.g. get_all_org_repos and the RepositoryViewSet.get_queryset view?

metashare/api/model_mixins.py

metashare/api/gh.py

jgerigmeyer · 2019-10-15T15:12:24Z

Chore: Clean up repository URL handling in Admin.

jgerigmeyer

@wlonk I have a number of ideas for cleanup here. At a high level view, I still think we only need admins to enter owner and name explicitly in the admin, then we use that to get the ID and only use the ID from there on out. I don't see a compelling reason to use (or store) the repo_url at all, since we're really only using it to parse out owner and name.

If you don't mind, I'd like to make a commit with my proposed cleanup here.

metashare/api/jobs.py

jgerigmeyer · 2019-10-15T21:22:29Z

@wlonk Okay, I'm sure there's some ugly code/architecture in my latest commit, but I'd be happy to talk through it with you and let you do whatever cleanup (or reverting) you like.

* master: Fix up tests to use current changes format Fix tests 📦 🛢 Various cleanup:

metashare/api/jobs.py

metashare/api/migrations/0002_githubrepository_product_productslug.py

jgerigmeyer · 2019-10-15T21:31:06Z

metashare/api/model_mixins.py

+        and get it from the model if present, or query the GitHub API as
+        a fallback, assuming that the current user can access the
+        current repo via the repo URL.
+        """


I don't love this deferring, but understand why it might be preferred. I wonder about (also?) trying to populate repo_id on every save?

jgerigmeyer · 2019-10-15T21:31:59Z

metashare/api/models.py

        User, on_delete=models.CASCADE, related_name="repositories"
    )
+    repo_id = models.IntegerField(unique=True)
+    repo_url = models.URLField()


The only reasons to store this IMO are 1) it's trivial, and 2) for ease of DB sleuthing and admin browsing.

metashare/api/models.py

jgerigmeyer · 2019-10-15T21:33:08Z

metashare/api/serializers.py

        )

+    def get_repo_url(self, obj) -> Optional[str]:
+        return f"https://github.com/{obj.repo_owner}/{obj.repo_name}"


I don't love this. We could store it on the model and populate it lazily (like we're doing for repo_id).

jgerigmeyer · 2019-10-15T21:33:28Z

metashare/api/serializers.py

+        repo_name = repo.repo_name
+        branch = obj.branch_name
+        if repo_owner and repo_name and branch:
+            return f"https://github.com/{repo_owner}/{repo_name}/tree/{branch}"


I also don't love these two hard-coded URLs, but it's not a big deal.

jgerigmeyer · 2019-10-15T21:35:03Z

metashare/api/views.py

+            try:
+                repo.get_repo_id(self.request.user)
+            except ResponseError:
+                pass


So I'd guess this is probably frowned upon? It forces an external API call in-band for any repos which don't have IDs yet -- on every get_queryset... 😬

Yeah, that's a little upsetting. Let's… see if it is a problem maybe?

This should only happen once ever per repo in the DB. So I would think in practice it won't be an issue? But let's discuss.

wlonk

Generally 👍 on your changes, though I left a few comments to discuss. Detailed discussion tomorrow?

metashare/api/jobs.py

metashare/api/migrations/0002_githubrepository_product_productslug.py

metashare/api/migrations/0030_replace_repo_url_with_repo_id.py

wlonk · 2019-10-15T22:34:04Z

metashare/api/views.py

+            try:
+                repo.get_repo_id(self.request.user)
+            except ResponseError:
+                pass


Yeah, that's a little upsetting. Let's… see if it is a problem maybe?

* master: Stick with 2 seconds. Copy semaphore logic from MetaDeploy. Use single Redis call to set expiry Rewrap some comments 🛢 Lint python. 📦 🧶 Combined dep upgrades. Bump pytest-django from 3.5.1 to 3.6.0 Add expire to semaphore 🛢 Delete new org from SF if initial flow run fails. Create orgs from latest task branch, not default repo branch.

metashare/api/models.py

jgerigmeyer · 2019-10-29T17:15:39Z

@wlonk @davisagli I'm satisfied with this PR, and did functional testing. As far as I'm concerned it's ready for review/merge.

wlonk

I can't formally approve, as I opened the PR, but I do approve. 💚

wlonk added 2 commits October 9, 2019 12:52

Allow GitHub with or without www. prefix

5622d51

All wubwubwub all the time now

a12d3ce

wlonk changed the title ~~Clean repository handling~~ [clean-repository-handling] Clean repository handling Oct 9, 2019

Merge branch 'master' into clean-repository-handling

828003a

davisagli approved these changes Oct 11, 2019

View reviewed changes

📦 Merge branch 'master' into clean-repository-handling

3147022

🛢 Store GH repos by ID, not URL

6cc9f5f

jgerigmeyer self-requested a review October 14, 2019 21:14

Merge branch 'master' into clean-repository-handling

d9a6a2b

* master:

jgerigmeyer requested changes Oct 14, 2019

View reviewed changes

metashare/api/model_mixins.py Outdated Show resolved Hide resolved

metashare/api/gh.py Outdated Show resolved Hide resolved

wlonk and others added 4 commits October 14, 2019 16:42

Use repo_id more consistently

7e46bcc

🛢 Unify repo_url field name

602e8c6

Merge branch 'master' into clean-repository-handling

813564a

Merge branch 'master' into clean-repository-handling

5609f7e

jgerigmeyer reviewed Oct 15, 2019

View reviewed changes

metashare/api/jobs.py Outdated Show resolved Hide resolved

metashare/api/jobs.py Outdated Show resolved Hide resolved

metashare/api/jobs.py Outdated Show resolved Hide resolved

📦 🛢 Attempt to use repo_id more consistently.

c8af371

🛢 Merge branch 'master' into clean-repository-handling

2d32b68

* master: Fix up tests to use current changes format Fix tests 📦 🛢 Various cleanup:

jgerigmeyer self-requested a review October 15, 2019 21:26

jgerigmeyer reviewed Oct 15, 2019

View reviewed changes

🛢 merge migrations

6098f81

wlonk commented Oct 15, 2019

View reviewed changes

🛢 Redo migrations to accept gradual repo-id backfilling

b4fb268

wlonk requested a review from jgerigmeyer October 23, 2019 19:21

wlonk and others added 3 commits October 23, 2019 13:49

📦 🧶 Merge branch 'master' into clean-repository-handling

5de53e6

Merge branch 'master' into clean-repository-handling

ace5db3

🛢 Add merge migration

ea6cee2

jgerigmeyer added 2 commits October 29, 2019 11:09

📦 Remove Pipfile

49ee9d1

jgerigmeyer reviewed Oct 29, 2019

View reviewed changes

metashare/api/models.py Outdated Show resolved Hide resolved

jgerigmeyer added 2 commits October 29, 2019 12:27

🛢 Clean up repo migrations.

7d28cbc

🛢 Remove GH repos in migration adding repo_id.

b4a9434

jgerigmeyer requested review from davisagli and jgerigmeyer October 29, 2019 17:08

jgerigmeyer approved these changes Oct 29, 2019

View reviewed changes

wlonk commented Oct 29, 2019

View reviewed changes

jgerigmeyer merged commit 7ddb697 into master Oct 29, 2019

jgerigmeyer deleted the clean-repository-handling branch October 29, 2019 17:50

jgerigmeyer mentioned this pull request Oct 29, 2019

Batch merge from forked OddBird repo. SFDO-Tooling/Metecho#284

Merged

Conversation

wlonk commented Oct 9, 2019

Uh oh!

wlonk commented Oct 14, 2019

Uh oh!

wlonk commented Oct 14, 2019

Uh oh!

jgerigmeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jgerigmeyer commented Oct 15, 2019

Uh oh!

jgerigmeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jgerigmeyer commented Oct 15, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wlonk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jgerigmeyer commented Oct 29, 2019

Uh oh!

wlonk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants