GitHub sync #1007

zacck · 2023-08-01T13:22:31Z

Notes for the reviewer

Draft pr for github sync, still some clean up to do but so far working

Related issue

Fixes #970

Review checklist

I have performed a self-review of my code
I have verified that all appropriate authorization policies have been implemented and tested
If needed, I have updated the changelog
Product has QA'd this feature

lib/lightning/version_control/github_client.ex

lib/lightning/version_control/project_repo.ex

lib/lightning_web/live/project_live/settings.ex

stuartc · 2023-08-03T10:16:05Z

@zacck looking like this is taking shape, we're going to need a lot more tests though - especially in the GithubClient and token stuff...

codecov · 2023-08-10T13:04:00Z

Codecov Report

Patch coverage: 81.48% and project coverage change: -0.12% ⚠️

Comparison is base (f25f5a8) 85.90% compared to head (950ba9d) 85.78%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1007      +/-   ##
==========================================
- Coverage   85.90%   85.78%   -0.12%     
==========================================
  Files         179      184       +5     
  Lines        5071     5206     +135     
==========================================
+ Hits         4356     4466     +110     
- Misses        715      740      +25

Files Changed	Coverage Δ
...ing_web/controllers/api/provisioning_controller.ex	`66.66% <0.00%> (-26.67%)`	⬇️
lib/lightning_web/live/project_live/settings.ex	`85.38% <67.92%> (-12.02%)`	⬇️
lib/lightning/version_control/github_client.ex	`94.59% <94.59%> (ø)`
lib/lightning/version_control/project_repo.ex	`100.00% <100.00%> (ø)`
lib/lightning/version_control/version_control.ex	`100.00% <100.00%> (ø)`
...ning_web/controllers/version_control_controller.ex	`100.00% <100.00%> (ø)`
...g_web/live/project_live/delete_connection_modal.ex	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

stuartc

Ran out of time for today - will pick up the rest of this first thing in the AM, for now I've left a few comments around the config/env changes.

stuartc · 2023-08-10T14:08:41Z

config/runtime.exs

+# If you've booted up with a SENTRY_DSN environment variable, use Sentry!
+config :sentry,
+  filter: Lightning.SentryEventFilter,
+  environment_name: config_env(),
+  included_environments:
+    if(System.get_env("SENTRY_DSN"), do: [config_env()], else: [])
+


Why is this here? Sentry is/was configured at the bottom of the file?

This is an error, didn't notice it before

stuartc · 2023-08-10T14:10:30Z

config/runtime.exs

+decoded_cert =
+  case github_cert do
+    :noset -> :notset
+    other -> Base.decode64!(other)
+  end


What is :noset? Why not default to nil? I don't see notset or noset used anywhere.

The intention was to leave a message for one to configure github, this can be achieved with nil as well will ammend

stuartc · 2023-08-10T14:11:48Z

.env.example

+#
+# Set this up to handle Github App configuration 
+GITHUB_APP_ID=12345 
+GITHUB_CERT_PATH=/path/to/cert/pem


Please change it to reflect the correct cert env variable.

If there is no default to set in the example then prepend the env vars with a hash.

I don't think there is any default we can provide for this.

CHANGELOG.md

stuartc · 2023-08-10T14:20:21Z

config/test.exs

+
+System.put_env("GITHUB_CERT", Base.encode64(cert))
+System.put_env("GITHUB_APP_ID", "111111")
+


Please don't use System.put_env, instead of implicitly coupling this to runtime.exs - rather use config :lightning, :github_app, ... and then have runtime.exs override only if ENV vars are present...

config/3 overrides conflicting keys on subsequent calls, so we can call it as many times as we want..

stuartc · 2023-08-14T13:28:55Z

Hey @zacck!

Systems/Flow/Design

It looks like you can only have one redirect url after a Github app installation, this is currently set to localhost:4000. This will pose problems with staging and production, in addition to different developers testing this locally.

Not without having a separate GitHub app for staging, production and development.

Opening a new tab and using a broadcast to update the settings page was an approach that was discussed - I'm not sure why it wasn't the road we took.

From looking around the documentation it appears that you can use a state query parameter when requesting a new application installation, by using that we can have the same experience as Google Sheets. See here and here. Anyway, given the current implementation this isn't necessary.

The webhook url is set to app.openfn.org. Which is correct, for production.

We should be using the webhook secret facility for our webhooks and validate the payload (see here)
We need to create a Github app for staging, production and dev each either the appropriate URLs for webhooks etc.
The Github apps settings point at the app domain + /, when Github does a POST to this path it will get a 404.

Are we using any of the Webhooks that Github sends?
Can we/should we disable webhooks for the app?

Code/Tests

No tests for VersionControlController
VersionControlController has no error handling in the event an installation id doesn't exist.
~~No tests for jwt token generation~~ [@stuartc done]
No tests for fetching/setting up a github connection/repo.
All GithubClient calls assume success, we need to ensure failures don't result in crashed LiveViews.

Features/UX

~~Show that Github integration is not available on a given installation~~ [@stuartc done]
~~The 'Stop Syncing' feature outlined in the spec doesn't appear to be implemented~~

We don't need this yet, because Syncing is manual, we do need to handle errors if the app is disconnected on the Github side.

No loading indicator when the app is fetching a list of repos, it's not in the spec but I think it's important to show we are waiting on a 3rd-party.
Modal for "Reinstall app"

I've made a bunch of small-ish changes, and rebase off main.

zacck · 2023-08-17T07:54:50Z

Hey @NickOpenFn @taylordowns2000 Here is how the click test looks at the moment

Screenshare.-.2023-08-17.9_52_08.AM.mp4

…state

- Remove tooltip - Test that repo/branch setup

Yaml via api

Run formatter, use provisioning permissions

stuartc · 2023-08-21T12:23:29Z

priv/repo/migrations/20230725063042_create_repo_project_connection.exs

+      add :project_id, references(:projects, type: :binary_id)
+      add :user_id, references(:users, type: :binary_id)


I think we need some kind of :on_delete behaviour here... @taylordowns2000 your thoughts?

So right now if a project is about to be deleted (or the user) it won't be possible because of this outstanding relationship.

Do we manage this in the delete/purge logic or add a nillify or delete_all cascade option in the schema?

good catch, let me know and I'll add it in

@zacck , please add an on_delete cascade for when a project is deleted and an on_delete nillify for when a user is deleted.

in other words: if the user who set the thing up is deleted, the PRC will stay. if the project is deleted, the PRC will also be deleted.

thank you!

stuartc · 2023-08-21T13:00:38Z

diff --git a/test/lightning/version_control/github_client_test.exs b/test/lightning/version_control/github_client_test.exs
index 1b63a5df5..9aaddb969 100644
--- a/test/lightning/version_control/github_client_test.exs
+++ b/test/lightning/version_control/github_client_test.exs
@@ -19,6 +19,9 @@ defmodule Lightning.VersionControl.GithubClientTest do
           "https://github.com/gitapi/app/installations/some-id/access_tokens" ->
             %Tesla.Env{status: 200, body: %{"token" => "some-token"}}
 
+          "https://github.com/gitapi/app/installations/invalid/access_tokens" ->
+            %Tesla.Env{status: 401, body: %{"message" => "Bad credentials"}}
+
           "https://github.com/gitapi/installation/repositories" ->
             %Tesla.Env{
               status: 200,
@@ -44,6 +47,11 @@ defmodule Lightning.VersionControl.GithubClientTest do
     test "client can fetch repo branches" do
       p_repo = insert(:project_repo)
 
+      assert {:ok, ["master"]} =
+               VersionControl.fetch_repo_branches(p_repo.project_id, p_repo.repo)
+
+      p_repo = insert(:project_repo, github_installation_id: "invalid")
+
       assert {:ok, ["master"]} =
                VersionControl.fetch_repo_branches(p_repo.project_id, p_repo.repo)
     end

@zacck check this out, we spoke about this the other day - it's really easy to get a cryptic error when the installation has been removed on the other side (or a failure of any kind):

  1) test Github Client client can fetch repo branches (Lightning.VersionControl.GithubClientTest)
     test/lightning/version_control/github_client_test.exs:47
     ** (ArgumentError) construction of binary failed: segment 2 of type 'binary': expected a binary but got: nil
     code: VersionControl.fetch_repo_branches(p_repo.project_id, p_repo.repo)
     stacktrace:
       (lightning 0.7.3) lib/lightning/version_control/github_client.ex:76: Lightning.VersionControl.GithubClient.build_client/1
       (lightning 0.7.3) lib/lightning/version_control/github_client.ex:27: Lightning.VersionControl.GithubClient.get_repo_branches/2
       (lightning 0.7.3) lib/lightning/version_control/version_control.ex:78: Lightning.VersionControl.fetch_repo_branches/2
       test/lightning/version_control/github_client_test.exs:56: (test)

When I intentionally change test/lightning_web/live/project_live_test.exs:402 to return a 401, the tests still pass but I can see a bunch of exceptions from the async task.

We need to handle these known sad paths, at the very least to stop server errors, but probably getting some kind of feedback to the user that something went wrong.

taylordowns2000 · 2023-08-21T16:32:16Z

hey @zacck , i spoke with Stu today and this is looking good. if you address his two points above (the on_delete stuff and this one) the recommendation would be to then merge this to main and iron out any additional changes/bugs/whatever once it's already deployed on demo.openfn.org (we do "edge" deployment, triggered by commits to the main branch.)

the hope is that this will be much easier to make the finishing touches on once it's running on a real server and we can see how it feels to hook up several projects to github.

sound good?

zacck force-pushed the github-sync branch from d35950d to 5bcf5c0 Compare August 3, 2023 07:14

stuartc reviewed Aug 3, 2023

View reviewed changes

lib/lightning/version_control/github_client.ex Show resolved Hide resolved

stuartc reviewed Aug 3, 2023

View reviewed changes

lib/lightning/version_control/project_repo.ex Outdated Show resolved Hide resolved

stuartc reviewed Aug 3, 2023

View reviewed changes

lib/lightning_web/live/project_live/settings.ex Outdated Show resolved Hide resolved

stuartc reviewed Aug 3, 2023

View reviewed changes

lib/lightning_web/live/project_live/settings.ex Outdated Show resolved Hide resolved

zacck force-pushed the github-sync branch 2 times, most recently from a288fa1 to 8401834 Compare August 8, 2023 12:41

zacck marked this pull request as ready for review August 8, 2023 12:56

stuartc reviewed Aug 10, 2023

View reviewed changes

stuartc force-pushed the github-sync branch from 65b0f45 to aba3248 Compare August 14, 2023 13:03

zacck force-pushed the github-sync branch 2 times, most recently from 0632f60 to 7e8b2ad Compare August 16, 2023 06:46

taylordowns2000 and others added 14 commits August 18, 2023 15:57

make project yaml available via API

7d99193

refactor into provisioning

c76972f

Build migration for project repos

59700b1

Add project_repos and test happy path

8de1c2c

Add Tesla to deps

2654d14

Add project repo connection schema

f916796

Add table for project repo connections

1330e19

Add token builder and http client for github

d9209e2

Add boundary module for version control

f920849

Add route, controller and live views for vcs settings

291a629

Some boundary tests for version control

8f877ae

Add conviniences to fetch from github and add to db with project_id

e2249be

Make settings hide and show version control connections according to …

c2dc2f0

…state

Scope routes as authenticated and setup redirect vcs controller

44a55be

zacck added 11 commits August 21, 2023 12:12

Liveview tests for github sync

841afbb

Clean up UI after review

4a52c0d

Move reinstall and delete connection button

bbbc51d

Fix code style

5344014

Bump version_control test coverage

3d0c5cb

Bump tests for settings.html

8d86012

Update state so repo and branch are visible after selection

860af9b

- Fix form button margin

6c6b5e9

- Remove tooltip - Test that repo/branch setup

Add styling for repo deletion modal

68de036

Delete connection and hide modal after

140fdc8

UI changes and button disable

f01272e

zacck force-pushed the github-sync branch from a5b9261 to f01272e Compare August 21, 2023 10:13

zacck and others added 3 commits August 21, 2023 12:13

Merge pull request #1035 from OpenFn/yaml-via-api

d403bff

Yaml via api

format and use provisioning permissions

2a54ef0

Merge pull request #1039 from OpenFn/permissions_for_zacck

e8bc649

Run formatter, use provisioning permissions

stuartc reviewed Aug 21, 2023

View reviewed changes

zacck added 3 commits August 22, 2023 09:47

Handle non success messages from github API

b796875

Test that non success statuses from github are handled

51d0465

Handle user and project deletion on project repo migration

115abe0

zacck force-pushed the github-sync branch from 9c2ce31 to 115abe0 Compare August 22, 2023 07:56

Test and code style fixes

ffc96fd

zacck force-pushed the github-sync branch from d59d463 to ffc96fd Compare August 22, 2023 08:06

zacck added 4 commits August 22, 2023 10:15

Remove unneccessary pattern matches

e4fb603

Only collect repos if we are setting up the github app else don't

6fb434f

Bump github client test coverage

94bcae8

Add client_payload for github fetch

950ba9d

taylordowns2000 merged commit cddf52f into main Aug 22, 2023
6 of 8 checks passed

taylordowns2000 deleted the github-sync branch August 22, 2023 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub sync #1007

GitHub sync #1007

zacck commented Aug 1, 2023 •

edited

Loading

stuartc commented Aug 3, 2023

codecov bot commented Aug 10, 2023 •

edited

Loading

stuartc left a comment

stuartc Aug 10, 2023

zacck Aug 11, 2023

stuartc Aug 10, 2023

zacck Aug 11, 2023

stuartc Aug 10, 2023

stuartc Aug 10, 2023

stuartc commented Aug 14, 2023 •

edited

Loading

zacck commented Aug 17, 2023

stuartc Aug 21, 2023

zacck Aug 21, 2023

taylordowns2000 Aug 21, 2023

stuartc commented Aug 21, 2023

taylordowns2000 commented Aug 21, 2023


		System.put_env("GITHUB_CERT", Base.encode64(cert))
		System.put_env("GITHUB_APP_ID", "111111")

		add :project_id, references(:projects, type: :binary_id)
		add :user_id, references(:users, type: :binary_id)

GitHub sync #1007

GitHub sync #1007

Conversation

zacck commented Aug 1, 2023 • edited Loading

Notes for the reviewer

Related issue

Review checklist

stuartc commented Aug 3, 2023

codecov bot commented Aug 10, 2023 • edited Loading

Codecov Report

stuartc 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

stuartc commented Aug 14, 2023 • edited Loading

zacck commented Aug 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartc commented Aug 21, 2023

taylordowns2000 commented Aug 21, 2023

zacck commented Aug 1, 2023 •

edited

Loading

codecov bot commented Aug 10, 2023 •

edited

Loading

stuartc commented Aug 14, 2023 •

edited

Loading