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

Fix typos in setup and docs #621

Merged
merged 5 commits into from
Jan 8, 2024
Merged

Conversation

keyserj
Copy link
Contributor

@keyserj keyserj commented Nov 8, 2023

Found a few issues when looking at the docs and running through the dev setup process. Happy to split these commits into separate PRs and/or remove some, just let me know.

The issues I ran into are these, and line up with the commits I made:

  • default port for serving the api didn't match the port that the UI was sending requests to. updated the .example.env file
  • setup commands require psql but that wasn't mentioned to be installed. I just added a mention to install psql - not sure if preference would be to add a specific install command like there is for node & yarn, but I also wasn't sure which command to add for this
  • some setup commands assume they're running from the package directory. I updated to assume running from root. could also consider adding a cd premiser-api for that section, or maybe mention to run from that directory.
  • the create-user:local command errored because zod validation failed. updated the fields to pass validation. I also searched createUser to see if there were tests to update, it looks like there aren't, but there are other createUser methods that use zod-invalid parameters (e.g. acceptedTerms instead of doesAcceptTerms), maybe this is just unfinished zod migration?
  • the docs look sweet. but the "edit this page" button directs to the main branch instead of master, which is invalid

Additionally, I ran check:everything locally and everything seems good except the howdju-service-common tests fail to create a test database (this occurs for every test in this package):

Started docker image 686746f6bede465550c8f08277e498382fc8a0a11e0646131326f8d4c4032817
Running: jest
 FAIL  lib/daos/PropositionsDao.test.ts (8.712 s)
  ● Console

    console.info
      Failed to create test DB howdju_cb10454714ebc85181e6c8171a589f4ed08add1b. Assuming it already exists and retrying a different name. Error: {"length":103,"name":"error","severity":"FATAL","code":"28P01","file":"auth.c","line":"333","routine":"auth_failed"}

I couldn't figure out how to get this going, and I tried looking in the test docs. It seems like the script change I made is only used for setup, so maybe these tests are irrelevant and/or GitHub Actions can run them for me? 😆

@carlgieringer
Copy link
Contributor

the howdju-service-common tests fail to create a test database

Yes, I think I am missing instructions to have a config/test.env file (it's used here). And actually I should probably just commit that to the repository so that no setup is required, but someone can modify it if they need to. The following contents should work:

DB_USER=postgres
DB_NAME=premiser
DB_PASSWORD=test-db-password
DB_HOST=localhost
DB_PORT=5433
LOG_LEVEL=silly
NODE_ENV=test

Regarding the Github actions, I need to figure out how to set them up for external PRs. They run automatically for PRs from branches in this repo, but I wasn't aware that they required additional config for PRs from other repos. I will spend a little time trying to figure it out, and otherwise we can just merge without those checks and deal with any breakages on master.

Thanks!

@keyserj
Copy link
Contributor Author

keyserj commented Jan 8, 2024

I'm not sure if this is specifically the case, but I know that generally without configuration, actions need to be approved when PRs are made from forks https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks#about-workflow-runs-from-public-forks. It might look different in this case because workflow run requests are also deleted after 30 days? I can try pushing again to see if another workflow run request is made that you can approve

Copy link
Contributor

@carlgieringer carlgieringer left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

docs/Development.md Outdated Show resolved Hide resolved
@carlgieringer
Copy link
Contributor

The CI checks failed. I think it's because the GH action is triggered by pull_request which doesn't provide access to this repo's secrets or vars. I can update the action not to refer to those.

@carlgieringer
Copy link
Contributor

Can you please rebase (now that #628 is merged) and see if the checks work? (I'm not so much concerned that your changes would break anything, but more interested in testing the process. I really appreciate your help!

@keyserj
Copy link
Contributor Author

keyserj commented Jan 8, 2024

I think it's because the GH action is triggered by pull_request which doesn't provide access to this repo's secrets or vars

Oh TIL this, hm. You'd think this should be acceptable if it's required to approve the workflow run anyway, but I guess this is how it is.

UI is looking for port 8082 by default

Signed-off-by: Joel Keyser <keyser.joel@gmail.com>
Signed-off-by: Joel Keyser <keyser.joel@gmail.com>
Signed-off-by: Joel Keyser <keyser.joel@gmail.com>
Signed-off-by: Joel Keyser <keyser.joel@gmail.com>
Signed-off-by: Joel Keyser <keyser.joel@gmail.com>
@carlgieringer
Copy link
Contributor

Great, the premerge checks passed. I will need to look more into the coverage check...it requires the GH token so that it can checkout the merge base, so I'm not sure it will work with pull_request.

@carlgieringer carlgieringer merged commit efd4be3 into Howdju:master Jan 8, 2024
4 of 5 checks passed
@carlgieringer
Copy link
Contributor

Thanks a ton, Joel!

Note to self: the DCO didn't seem to quote be recognized correctly by GH. The DCO check passed, and manually inspecting the commits shows that the correct sign off message is there, but Github still complained that "The base branch requires all commits to be signed".

Screenshot 2024-01-08 at 10 38 02 AM

@keyserj keyserj deleted the fix-typos branch January 8, 2024 18:46
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