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

[Documentation] RB refresh script instructions added to RB Readme #6535

Conversation

AlexandraLivadas
Copy link
Contributor

Instructions on using the /tools/raisinbread_refresh.php script to source RB data added to the RaisinBread README for the 23.0-release.

@AlexandraLivadas AlexandraLivadas added 23.0.0-testing Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) Documentation PR or issue introducing/requiring modifications to the code documentation (test plans, wikis, docs) labels May 14, 2020
Copy link
Contributor

@christinerogers christinerogers 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 @AlexandraLivadas

will request a review from rida/john/dave

@christinerogers christinerogers added the Passed Manual Tests PR has undergone proper testing by at least one peer label May 14, 2020
Copy link
Collaborator

@cmadjar cmadjar left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ridz1208 ridz1208 self-assigned this May 15, 2020
@@ -29,6 +29,16 @@ that is not the case, replace all `mysql` commands below by the necessary values
`mysql -u user -p -h host database_name`*

##### Sourcing SQL
The script `tools/raisinbread_refresh.php` includes functionality that drops all existing tables in the
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm okai with this addition however I don't think it belongs under the Sourcing SQL header. This script does so much more than just sourcing the SQL. It actually replaces pretty much this entire readme.

I think we should instead divide the Installing RB header into 2 sections. automated and manual instructions and make it clear to users that you should pick or the other to install it.

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 can make this change to the formatting. For developers who are first sourcing RB though, they also need to make the steps in the following section "Configuring", which isn't done by this script. I'll try to make this clear.

root directory and run the following command:

```bash
./tools/raisinbread_refresh.php
Copy link
Contributor

Choose a reason for hiding this comment

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

The script can also be run using make testdata which is probably a little more clear.

@@ -29,6 +29,16 @@ that is not the case, replace all `mysql` commands below by the necessary values
`mysql -u user -p -h host database_name`*

##### Sourcing SQL
The script `tools/raisinbread_refresh.php` includes functionality that drops all existing tables in the
database and sources all of the RaisinBread data automatically. To run this script, navigate to the LORIS
Copy link
Contributor

Choose a reason for hiding this comment

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

I think ti would be useful to add a note here to the effect of "The script will preserve database settings such as API Keys and server configuration settings to simplify the process of switching between development environments.

@AlexandraLivadas
Copy link
Contributor Author

Due to the above linked PR by @johnsaigle , I will remove the latest change in the Readme that instructs users to install instruments themselves when using the raisinbread_refresh.php tool.

```

If RaisinBread is being installed for the first time, the steps outlined below in the
[Configuring](https://github.com/aces/Loris/tree/23.0-release/raisinbread#configuring) section must be completed.
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to not use a hyperlink here because we'll have to update the version info in the link every release.

raisinbread/README.md Outdated Show resolved Hide resolved
Co-authored-by: John Saigle <4022790+johnsaigle@users.noreply.github.com>
Copy link
Contributor

@christinerogers christinerogers left a comment

Choose a reason for hiding this comment

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

thanks for these important updates for 23!

```

If RaisinBread is being installed for the first time, the steps outlined below in the
_Configuring_ section must be completed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

since "Configuring" is a header, you could use a in-file link to it instead of italics

@driusan driusan merged commit 99fb559 into aces:23.0-release Jun 1, 2020
@ridz1208 ridz1208 added this to the 23.0.0 milestone Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) Documentation PR or issue introducing/requiring modifications to the code documentation (test plans, wikis, docs) Passed Manual Tests PR has undergone proper testing by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Documentation] mention RB refresh script in RB readme
6 participants