Minor updates to LSQL and LDMS modules. #127
Conversation
Minor alterations to the LDMS workshop instructions to make it easier to follow.
tebanieo
left a comment
tebanieo
left a comment
There was a problem hiding this comment.
Thanks For the update, I have requested one modification, and provided one NIT.
Thanks!
| ```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.
Change to
PATH="~/.local/bin:$PATH"
This will ensure portability across different distros/images.
There was a problem hiding this comment.
Update applied. Thanks.
| 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.
NIT: Add spaces in between the warnings for better readibility.
There was a problem hiding this comment.
Update applied. Thanks.
switch180
left a comment
switch180
left a comment
There was a problem hiding this comment.
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.
| title: "Create DMS Resources" | ||
| menuTitle: "Create DMS Resources" | ||
| date: 2021-04-25T07:33:04-05:00 | ||
| weight: 25 |
There was a problem hiding this comment.
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.
|
|
||
| ```bash | ||
| sudo pip3 install chalice mysql-connector-python | ||
| pip3 install chalice mysql-connector-python |
There was a problem hiding this comment.
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.
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. |
3 participants
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.