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.

Sign up for GitHub

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

Jump to bottom

Fixes #3692 by adding database info to environment-setup file #3722

Merged
merged 4 commits into from
Oct 26, 2022
Merged

Fixes #3692 by adding database info to environment-setup file #3722

merged 4 commits into from
Oct 26, 2022

Conversation

SerpentBytes
Copy link
Contributor

@SerpentBytes SerpentBytes commented Oct 20, 2022 •
edited
Loading

Issue This PR Addresses

Fix #3692 by including information on properly seeding the database in the environment-setup file.

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

This PR adds information on how to seed PostgresSQL locally for testing purposes properly.

Steps to test the PR

  • check the updated environment-setup file.

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@gitpod-io
Copy link

gitpod-io bot commented Oct 20, 2022

@manekenpix manekenpix added type: documentation (docs) Improvements or additions to documentation hacktoberfest-accepted labels Oct 20, 2022
@SerpentBytes
Copy link
Contributor Author

@manekenpix I filed the issue and created the PR. Please assign someone else to review this PR.

@RC-Lee
Copy link
Contributor

RC-Lee commented Oct 20, 2022

You're being assigned the PR, meaning you're responsible for this PR, not being assigned to review.

Also, this
image
I'm guessing is most likely from not rebasing correctly.
If rebased right, the message would not appear, and there would only be 1 commit to this PR.

@SerpentBytes
Copy link
Contributor Author

@RC-Lee, Sorry, my bad. :)

manekenpix
manekenpix requested changes Oct 20, 2022
View reviewed changes
src/web/docusaurus/docs/getting-started/environment-setup.md Outdated

The first time you start the database, it doesn't have any data. Therefore, we need database seeding as a way to populate the database with an initial set of data to make some parts of our application work properly.

To seed the database with initial data use the command `pnpm seed`.
Copy link
Member

Choose a reason for hiding this comment

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

I think what you need to run is pnpm db:init, which runs db:migrate and db:seed. It'd be a good idea to explain what those 2 commands do, and also it's very likely that, after running pnpm db:init the parse container needs to be restarted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working on it.

@SerpentBytes SerpentBytes force-pushed the issue-3692 branch 2 times, most recently from 16986fb to 81c5cb9 Compare October 20, 2022 19:08
@RC-Lee
Copy link
Contributor

RC-Lee commented Oct 20, 2022

I do my setup a bit out of the status quo, but I'm wondering if you've tested yourself? (Including deleting the redis cache (redis-data) and starting from scratch)
Another good way to have this tested is if you could ask one of your current classmates to go through it line by line and see if they can get Telescope started up.

Just from what I've experienced, the db scripts will not be able to run correctly if the db containers aren't running.
Thought it'd be worth pointing out.

@SerpentBytes
Copy link
Contributor Author

If you do the setup using the environment-setup as a guide, you would most likely run into database issues. I pointed out this problem in the Telescope channel, and @humphd suggested filing an issue for it.

manekenpix
manekenpix requested changes Oct 20, 2022
View reviewed changes
src/web/docusaurus/docs/getting-started/environment-setup.md Outdated

Running this command will populate the database with an initial set of data to make some parts of our application work properly.

Finally, restart the `parser` container by running `pnpm:services start parser`/
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the forward slash at the end of this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a typo, sorry.

src/web/docusaurus/docs/getting-started/environment-setup.md Outdated
@@ -240,6 +240,24 @@ Docker builds Telescope's dependencies at launch and keeps them on disk. In some

:::

### Seeding PostgresSQL
Copy link
Member

Choose a reason for hiding this comment

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

I think this section should be part of the Start telescope section since the pnpm db:init script (not a command), won't work unless telescope, and specifically the db containers, are already running. Also, I think the env var in this env file needs to be present in the environment.

Also, make sure your instructions work before pushing your changes to this PR.

Copy link
Contributor Author

@SerpentBytes SerpentBytes Oct 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Also, make sure your instructions work before pushing your changes to this PR.

Definitely.

@AmasiaNalbandian
Copy link
Contributor

How is progress on this? I just ran into this issue and this would be a nice to have!

@AmasiaNalbandian AmasiaNalbandian added the type: nice to have Feature that'd be nice to have, but not a priority label Oct 26, 2022
@SerpentBytes
Copy link
Contributor Author

How is progress on this? I just ran into this issue and this would be a nice to have!

I am working on this.

@AmasiaNalbandian
Copy link
Contributor

These steps did not seem to work for the db...

@SerpentBytes
Copy link
Contributor Author

These steps did not seem to work for the db...

Yes, because it's WIP. In the meantime, users can setup their environment using steps not introduced through this PR.

@SerpentBytes SerpentBytes force-pushed the issue-3692 branch 2 times, most recently from 4840906 to cbb0eff Compare October 26, 2022 20:22
manekenpix
manekenpix approved these changes Oct 26, 2022
View reviewed changes
Copy link
Member

@manekenpix manekenpix 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 @SerpentBytes !

@manekenpix manekenpix merged commit bd8e604 into Seneca-CDOT:master Oct 26, 2022
@SerpentBytes
Copy link
Contributor Author

@manekenpix Thank you for all your help.

@SerpentBytes SerpentBytes deleted the issue-3692 branch October 27, 2022 00:56
liutng pushed a commit to liutng/telescope that referenced this pull request Nov 21, 2022
@SerpentBytes @liutng
Fixes Seneca-CDOT#3692 by adding database info to environment-setup…
70b60e0 
… file (Seneca-CDOT#3722)

* Fixes Seneca-CDOT#3692 by adding database info to docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manekenpix manekenpix manekenpix approved these changes

Assignees

@SerpentBytes SerpentBytes

Labels
hacktoberfest-accepted type: documentation (docs) Improvements or additions to documentation type: nice to have Feature that'd be nice to have, but not a priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update environment-setup file with info on how to setup Postgres
4 participants
@SerpentBytes @RC-Lee @AmasiaNalbandian @manekenpix