-
Notifications
You must be signed in to change notification settings - Fork 123
Minor updates to LSQL and LDMS modules. #127
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
Minor updates to LSQL and LDMS modules. #127
Conversation
Minor alterations to the LDMS workshop instructions to make it easier to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks For the update, I have requested one modification, and provided one NIT.
Thanks!
@@ -47,7 +47,8 @@ cd workshops/relational-migration | |||
4. Next, run this to install three components: Boto3 (AWS SDK for Python), Chalice, and the MySQL connector for Python. | |||
|
|||
```bash | |||
sudo pip3 install chalice mysql-connector-python | |||
pip3 install chalice mysql-connector-python | |||
export PATH="/home/ubuntu/.local/bin:$PATH" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to
PATH="~/.local/bin:$PATH"
This will ensure portability across different distros/images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update applied. Thanks.
@@ -31,7 +31,8 @@ To explore the dataset, follow the instructions below to log in to the EC2 serve | |||
6. You can see all the 6 files copied from the IMDB dataset to the local EC2 directory. | |||
 | |||
7. Feel free to explore the files. | |||
8. Go to AWS CloudFormation [Stacks](https://console.aws.amazon.com/cloudformation/home?region=us-east-1#/stacks?filteringStatus=active&filteringText=&viewNested=true&hideStacks=false) and click on the stack you created earlier. Go to the Parameters tab and copy the username and password listed next to "DbMasterUsername" and "DbMasterPassword". | |||
8. If you are completing this workshop at an AWS hosted event, go to [AWS CloudFormation Console](https://console.aws.amazon.com/cloudformation/home#/stacks?filteringStatus=active&filteringText=&viewNested=true&hideStacks=false) and select the stack named **ddb**. Go to the **Parameters** tab and copy the username and password listed next to **DbMasterUsername** and **DbMasterPassword**. | |||
::alert[_If you are completing this workshop in your AWS account copy the **DbMasterUsername** and **DbMasterPassword** from the CloudFormation stack used to configure the MySQL environment._] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Add spaces in between the warnings for better readibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update applied. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hold off on merging this until Aman syncs his changes to the relational lab. No action needed on your part, but please hold off on merge.
@@ -5,23 +5,23 @@ date: 2021-04-25T07:33:04-05:00 | |||
weight: 25 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please hold off on merging until Aman has a chance to sync his changes to the labs. He did a direct commit on the internal version, and these changes might conflict on merge.
@@ -47,7 +47,8 @@ cd workshops/relational-migration | |||
4. Next, run this to install three components: Boto3 (AWS SDK for Python), Chalice, and the MySQL connector for Python. | |||
|
|||
```bash | |||
sudo pip3 install chalice mysql-connector-python | |||
pip3 install chalice mysql-connector-python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this works without sudo? without error? Our concern is that the sudo is usually on the pip3 install because the user can't install packages. If you remove sudo, I assume you need to add --user
to the command. Please explain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified: This breaks the lab. Rejecting this change.
The merge of the internal diffs to the master branch are complete. Now, I talked to Rob and he and I have concerns about the removal of sudo on the pip3 command. Can you respond to that specific comment? Once that's resolved, we can squash and merge. |
The problem was with the second line. The initial change installs pip then updates the python path. Changing the the update path statement from I made the original change because using pip3 install chalice mysql-connector-python This works and is in line with best practice. |
Hi! I rejected it because it uses the export and that would be lost on reboot. Changes to the PATH need to be done in the shell's profile file, such as the .bashrc file. In line with other workshops, this should be done in the instance bootstrap step via the userdatac9.sh script. Your change didn't use that method. These instances can be rebooted or stopped as they run on C9. While sudo pip3 install is not ideal, it works and it's one line. I was able to run chalice fine after the sudo pip3 install command with no changes to the path, hence the export is removed from the merged commit. I didn't do the rest of the lab, assuming that the working chalice command is sufficient. |
Updated the git hash to use in LSQL module so the chalise app is not tied to the us-west-2 region.
Minor alterations to the LDMS module instructions to make it easier to follow.
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.