Some candidate areas for review

gmerritt commented 7 months ago

Dear RTL Developers,

Here are several areas of this application that might benefit from examination & feedback. There may be plenty more, but these top the list from what I know and understand.

(1) The python “GCP” bits that give the app its specific function
(2) Use of AWS Secrets Manager...
- Instead of local secrets for development
- Instead of S3 in deployment
(3) The app’s main Vue page
(4) The UI / the tool / its use, overall

(1) The python “GCP” bits that give the app its specific function

https://github.com/ets-berkeley-edu/hartsfield/blob/main/hartsfield/api/datahub_archive_url_fetch.py

Its role:

Clean & check submitted gs:// url
Instantiate GCP storage client
Ask for…
- …a 7-day “secret” https:// download URL…
- …for the given gs:// blob reference.
Return response & status message, whether good or bad

(2) Use of AWS Secrets Manager

The setup

Secrets defined in AWS Secrets manager
- AUTHORIZED_USERS, GCP_JSON_CREDENTIALS
- https://us-west-2.console.aws.amazon.com/secretsmanager/listsecrets?region=us-west-2
Environment variables referenced in hartsfield / config / default.py
- https://github.com/ets-berkeley-edu/hartsfield/blob/main/config/default.py
Utility python to fetch a Secrets value
- https://github.com/ets-berkeley-edu/hartsfield/blob/main/hartsfield/api/read_aws_secret.py

Instead of local secrets for development

https://ucberkeley.awsapps.com/start/#/?tab=accounts -> Access keys

Run the following commands in your terminal to set the AWS environment variables. Learn more

export AWS_ACCESS_KEY_ID="AS…
export AWS_SECRET_ACCESS_KEY="dWwo…
export AWS_SESSION_TOKEN="IQoJ…

Instead of S3 in deployment

EB service role
- RTL-app-hartsfield-dev-secrets-read
- https://us-east-1.console.aws.amazon.com/iam/home?region=us-west-2#/roles/details/RTL-app-hartsfield-dev?section=permissions

(3) The app’s main Vue page

Are the formatting and functionality declared & coded appropriately for a Vue page?

https://github.com/ets-berkeley-edu/hartsfield/blob/main/src/views/Fetchurl.vue

(4) The UI / the tool / its use, overall

As simple as it is, does it present and behave reasonably and according to convention?

Requires campus full tunnel VPN to access, and requires your UID to be allow-listed to authenticate. (Greg can add UIDs for review purposes.)

https://hartsfield-dev.datahub.berkeley.edu/

gmerritt commented 7 months ago

Hey there, everyone!

Shane told me that @remillet suggested that I put some candidate areas of review for Hartsfield into a GitHub issue, so here it is!

I'll also tag @pauline2k and @lyttam for reference, but I'm not sure the precise process here regarding just who / when / how / etc.

If additional and/or different information from me would be helpful, just holler.

Thanks!

pauline2k commented 7 months ago

Thanks @gmerritt. We'll have more devs on hand next week to take a look. Probably what I'll do is set up a workflow with a placeholder PR, like the one described here, so that we can make use of our usual code-commenting practices https://astrofrog.github.io/blog/2013/04/10/how-to-conduct-a-full-code-review-on-github/

gmerritt commented 7 months ago

Note -- re: the astrofrog suggestion for code review (Good stuff! Thanks Pauline!), it seems to me that the first approach, which requires the first commit to be unimportant, is probably not our case.

This puts us in the "If the first commit is important..." case, in which...

... this makes things a little more complicated. The approach we'll take here is to create two new branches - review, containing the code to review, and empty, containing no files - both of which contain a common and empty first commit (which we will add). In this way, the two branches have a common history, even though the empty branch has no files. We can set then set up a pull request from review to empty.

It seems to all break down to a straightforward set of steps, but definitely something to not be done lightly, and with some care & precautions.

I'm willing to jump in on this, but would be happy to know if there are any particularly suggestions or pieces of advice related to this process. Also, could be something to do as a pair of people screen sharing over Zoom, just for a second brain to be involved. :)

pauline2k commented 7 months ago

@gmerritt I think the initial commit might have been placeholder enough that, for review purposes, we might be able to get by with creating a new PR for all subsequent commits. Anyhow I've gone ahead with creating https://github.com/ets-berkeley-edu/hartsfield/pull/30, and we'll see how the workflow suits us!

gmerritt commented 7 months ago

Awesome! Thank you, @pauline2k !

johncrossman commented 7 months ago

@gmerritt @pauline2k - I'm jumping in the mix here – I'll put comments to PR #30.

gmerritt commented 7 months ago

Hi @johncrossman and @pauline2k,

Is this a reasonable (or preferred...or discouraged...) way for me to work my way through implementing changes?

I start with a local, current Hartsfield (matching what’s live, not John’s suggestions)
I begin implementing & testing locally some of the suggested changes
I make a new PR with my first set of several changes
I (or John) approves PR, I watch Hartsfield redeploy, confirm that it’s working ok in AWS
I lather, rinse, repeat with my subsequent changes

Thanks!

pauline2k commented 7 months ago

@gmerritt yes, exactly. PR #30 is just a reference point, so you want to submit your actual changes as separate PRs against the current main branch.

ets-berkeley-edu / hartsfield