Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: setup for running checks locally #29

Merged
merged 4 commits into from
Oct 1, 2021
Merged

feat: setup for running checks locally #29

merged 4 commits into from
Oct 1, 2021

Conversation

mds1
Copy link
Contributor

@mds1 mds1 commented Sep 30, 2021

See README.md for details on the new environment variable configuration

  • Replace INFURA_API_KEY with RPC_URL to remove Infura dependence (need to change this in repo secrets for CI to pass)
  • Write to reports folder when running locally

@mds1 mds1 requested a review from moodysalem September 30, 2021 17:24
@mds1 mds1 mentioned this pull request Sep 30, 2021

} else {
// Running in CI, save to file on REPORTS_BRANCH
const { github } = await import("./utils/clients/github"); // lazy load to avoid errors about missing env vars when not in CI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would probably do this slightly differently, where you just have a different entry point for main and local testing, but you can probably also just use something like is the github token to enter this code path

also would be cool if our testing was scripted (integration testing) for ongoing maintenance, but that's a lot of work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would probably do this slightly differently, where you just have a different entry point for main and local testing, but you can probably also just use something like is the github token to enter this code path

Yea different entry point could work also, but probably would want to pull out some of the existing code into standalone methods to avoid duplicating code. My thinking was this is the shortest path forward to knocking out the bigger unknowns (e.g. tx simulation), then could refactor later

use something like is the github token to enter this code path

Do you mean remove the RUNNING_LOCALLY flag and use the presence of the GitHub API keys in its place? Can do that, but do you know what env vars to check for? new Octokit() threw with a missing GITHUB_ACTION env var and I didn't see that defined anywhere in the CI config, so that was confusing

also would be cool if our testing was scripted (integration testing) for ongoing maintenance, but that's a lot of work

Could you create an issue with some more details on what you had in mind here?

Copy link
Contributor

@moodysalem moodysalem Sep 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea different entry point could work also, but probably would want to pull out some of the existing code into standalone methods to avoid duplicating code. My thinking was this is the shortest path forward to knocking out the bigger unknowns (e.g. tx simulation), then could refactor later

agree, you would want both entry points to call into the report generator code, where the cli entry point writes the output to console, (or somewhere useful for manual testing), takes cli arguments for which governor and/or proposal to check, etc.
and the ci entry point saves report results to the repo

it shouldn't be a large refactor, but i'm ok with doing it later

Do you mean remove the RUNNING_LOCALLY flag and use the presence of the GitHub API keys in its place?

yeah, you only need to check the one GITHUB_TOKEN i think

created #31

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool that all makes sense, thanks 👌

@@ -1,3 +1,3 @@
node_modules
.idea

reports/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add .env.local maybe

Copy link
Contributor Author

@mds1 mds1 Sep 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, must have been using my global gitignore. Done in 6bd5777

@mds1 mds1 merged commit adb959d into main Oct 1, 2021
@mds1 mds1 deleted the local-setup branch October 1, 2021 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants