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

BCDA-8350: Add workflow to generate database docs #994

Merged
merged 14 commits into from
Sep 26, 2024
Merged

BCDA-8350: Add workflow to generate database docs #994

merged 14 commits into from
Sep 26, 2024

Conversation

kyeah
Copy link
Contributor

@kyeah kyeah commented Sep 23, 2024

🎫 Ticket

https://jira.cms.gov/browse/BCDA-8350

🛠 Changes

ℹ️ Context

As part of BCDA-8350, I was interested in an easier way to keep the documentation up to date. Although ERDs can be generated manually by tooling like pgAdmin, it would be preferable for devs to have the diagrams generated automatically in a central location.

We can still pull the main ERDs into confluence, but instead of making them ourselves we can download them directly from the PR after merging.

🧪 Validation

See dbdocs in this PR.

@kyeah kyeah changed the title Kev/dbdocs BCDA-8350: Add workflow to generate database docs Sep 23, 2024
@kyeah kyeah requested a review from a team September 23, 2024 19:20
Copy link
Contributor

@carlpartridge carlpartridge left a comment

Choose a reason for hiding this comment

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

Do suppresions have one suppression_files (file_id?)?

The foreign_key connection is a bit wonky on the schema.svg but overall this seems great.

My understanding is that this DB is shared with SSAS which would include tables like systems and groups. Do you think it is problematic (or fortunate) that those are missing here?

Makefile Outdated Show resolved Hide resolved
run: |
echo $VAULT_PW > .vault_password
bash ops/secrets --decrypt
mv -fv shared_files/encrypted/* shared_files/decrypted/
Copy link
Contributor

Choose a reason for hiding this comment

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

These github actions dont leave any kind of artifacts anywhere I assume?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh hm! I don't think so, I copied this from the existing ci-workflow.yml. We're good to keep decrypted secrets on the public GA machines, right @CMSgov/ab2d-bcda-dpc-platform?

Copy link
Member

Choose a reason for hiding this comment

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

Better not to leave them lying around if we can avoid it. There was a vulnerability found recently in GitHub Actions artifacts. That doesn't apply directly here, but does point toward the potential for leaks.

.github/workflows/dbdocs.yml Outdated Show resolved Hide resolved
.github/workflows/dbdocs.yml Outdated Show resolved Hide resolved
.github/workflows/dbdocs.yml Outdated Show resolved Hide resolved
@carlpartridge
Copy link
Contributor

carlpartridge commented Sep 23, 2024

Seeing a failure on generate-docs, I think you need to add a --force to it to make sure it is overwriting the existing docs (https://github.com/k1LoW/tbls?tab=readme-ov-file#re-generating-database-documentation)?

@kyeah
Copy link
Contributor Author

kyeah commented Sep 23, 2024

Do suppressions have one suppression_files (file_id?)?

yup, each suppression is from a single file! subsequent changes for a single MBI have different records with different file_ids, so we grab the one with the latest timestamp.

The foreign_key connection is a bit wonky on the schema.svg but overall this seems great.

yeah, i didn't see any configuration to either hide the text or make it prettier.

My understanding is that this DB is shared with SSAS which would include tables like systems and groups. Do you think it is problematic (or fortunate) that those are missing here?

oops yes! I forgot to run the SSAS table migrations (those live in the bcda-ssas-app repo.) Hmm that also makes autogeneration difficult — let me look at the best way to do that (I think maybe add a separate workflow to generate docs just for the SSAS app within the other repo?)

@carlpartridge
Copy link
Contributor

My understanding is that this DB is shared with SSAS which would include tables like systems and groups. Do you think it is problematic (or fortunate) that those are missing here?

oops yes! I forgot to run the SSAS table migrations (those live in the bcda-ssas-app repo.) Hmm that also makes autogeneration difficult — let me look at the best way to do that (I think maybe add a separate workflow to generate docs just for the SSAS app within the other repo?)

If the BCDA doesnt "know" about systems and groups, etc then to some degree it makes sense to leave them out of the bcda-app repo documentation. We can pretty easily recreate these docs in bcda-ssas-app, which would have the other side of things (unless ssas references bcda-app tables?). Still a bit awkward to have one DB docs spread out across two repos.
Overall it feels a bit messy to have multiple apps reaching into the same DB, especially migrating structure and data.

@kyeah
Copy link
Contributor Author

kyeah commented Sep 24, 2024

Yeah, I think lets keep the docs separate and we can mentally treat them as two separate databases since that's how they operate if i recall correctly. The fact that they actually live in the same database is more of an odd implementation detail 🙃

@kyeah kyeah marked this pull request as ready for review September 24, 2024 17:36
.github/workflows/dbdocs.yml Outdated Show resolved Hide resolved
Co-authored-by: Sean Fern <seanfern@navapbc.com>

- name: Cleanup secrets
if: always()
run: rm -r shared_files/decrypted
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to rm .vault_password and maybe shared_files/encrypted as well?

Copy link
Contributor

@carlpartridge carlpartridge left a comment

Choose a reason for hiding this comment

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

LGTM but you might want to get Sean's sign off as well.

kyeah added a commit to CMSgov/bcda-ssas-app that referenced this pull request Sep 26, 2024
## 🎫 Ticket

https://jira.cms.gov/browse/BCDA-8350

## 🛠 Changes

- Adds database documentation in the repo using
https://github.com/k1LoW/tbls
- Adds github workflow to automatically update PRs that touch the
migrations folder

## ℹ️ Context

This is the sister PR to CMSgov/bcda-app#994.

## 🧪 Validation

See dbdocs in this PR.
@kyeah kyeah merged commit e793c1b into main Sep 26, 2024
6 checks passed
@kyeah kyeah deleted the kev/dbdocs branch September 26, 2024 15:39
laurenkrugen-navapbc pushed a commit that referenced this pull request Oct 3, 2024
## 🎫 Ticket

https://jira.cms.gov/browse/BCDA-8350

## 🛠 Changes

- Adds database documentation in the repo using
https://github.com/k1LoW/tbls
- Adds github workflow to automatically update PRs that touch the
migrations folders

## ℹ️ Context

As part of BCDA-8350, I was interested in an easier way to keep the
documentation up to date. Although ERDs can be generated manually by
tooling like pgAdmin, it would be preferable for devs to have the
diagrams generated automatically in a central location.

We can still pull the main ERDs into confluence, but instead of making
them ourselves we can download them directly from the PR after merging.

## 🧪 Validation

See dbdocs in this PR.

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Sean Fern <seanfern@navapbc.com>
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.

3 participants