Fix script for requesting phone number on staging / production #1551
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -17,7 +17,7 @@ printf " WARNING!!!!\n" | |||
| printf " This will delete all but one long code from a Pinpoint pool!\n" | |||
| printf " You do not want to run this or production!\n" | |||
| printf "\n------------------------------------------------------------\n" | |||
| printf "Are you sure you want to continue?" | |||
| printf "Are you sure you want to continue?\n" | |||
| echo -n "If so, type 'drain'> " | |||
There was a problem hiding this comment.
Consider using read -p to combine the prompt and read into a single line for better readability:
read -p "If so, type 'drain'> " check
There was a problem hiding this comment.
yes this is definitely where the AI review is useful!
Slightly odd that it didn't flag the same code in the other file
| @@ -26,15 +26,15 @@ if [ "$check" != "drain" ]; then | |||
| exit 1 | |||
| fi | |||
|
|
|||
| if aws pinpoint-sms-voice-v2 describe-pools --pool-ids $1; then | |||
| if aws pinpoint-sms-voice-v2 describe-pools --pool-ids $1 > /dev/null 2>&1; then | |||
There was a problem hiding this comment.
Redirecting both stdout and stderr to /dev/null can make debugging difficult if something goes wrong. Consider logging errors to a file or displaying them conditionally.
There was a problem hiding this comment.
I thought of that but figured 🤷 but tbh I just want to hide the output when it's successful, so we can do that (but not hide the errors) by getting rid of the 2>&1.
| numbers=$(aws pinpoint-sms-voice-v2 list-pool-origination-identities --pool-id $1 | jq -r ".OriginationIdentities[].OriginationIdentity") | ||
| read -ra numbersArray <<< $numbers # Split the string into an array | ||
|
|
||
| echo "Found ${#numbersArray[@]} numbers in pool $1. Releasing all but one." | ||
| for number in ${numbersArray[@]:1}; do # Skip the first number - have to keep at least one number in the pool | ||
| echo "Releasing $number..." | ||
| aws pinpoint-sms-voice-v2 disassociate-origination-identity --iso-country-code CA --pool-id $1 --origination-identity $number | ||
| aws pinpoint-sms-voice-v2 release-phone-number --phone-number-id $number | ||
| aws pinpoint-sms-voice-v2 disassociate-origination-identity --iso-country-code CA --pool-id $1 --origination-identity $number > /dev/null 2>&1 |
There was a problem hiding this comment.
Redirecting both stdout and stderr to /dev/null can make debugging difficult if something goes wrong. Consider logging errors to a file or displaying them conditionally.
| aws pinpoint-sms-voice-v2 disassociate-origination-identity --iso-country-code CA --pool-id $1 --origination-identity $number | ||
| aws pinpoint-sms-voice-v2 release-phone-number --phone-number-id $number | ||
| aws pinpoint-sms-voice-v2 disassociate-origination-identity --iso-country-code CA --pool-id $1 --origination-identity $number > /dev/null 2>&1 | ||
| aws pinpoint-sms-voice-v2 release-phone-number --phone-number-id $number > /dev/null 2>&1 |
There was a problem hiding this comment.
Redirecting both stdout and stderr to /dev/null can make debugging difficult if something goes wrong. Consider logging errors to a file or displaying them conditionally.
| echo "Pool $2 does not exist" | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
The check for the existence of the pool has been removed. This might lead to issues if the pool ID provided does not exist. Consider re-adding the validation for the pool ID.
There was a problem hiding this comment.
yeah I moved it below but commented it out and forgot to uncomment.
| printf "\n------------------------------------------------------------\n" | ||
| printf " WARNING!!!!\n" | ||
| printf " This will add new phone numbers to a Pinpoint pool\n" | ||
| printf " You might not want to run this in production!\n" | ||
| printf "\n------------------------------------------------------------\n" | ||
| printf "Are you sure you want to continue?" | ||
| printf "Are you sure you want to continue?\n" |
There was a problem hiding this comment.
The newline character \n added at the end of the printf statement is a good change for formatting. Ensure that this change is consistent across all similar printf statements in the script.
There was a problem hiding this comment.
It is consistent...
| @@ -41,24 +37,17 @@ if [ "$check" != "request" ]; then | |||
| exit 1 | |||
| fi | |||
|
|
|||
| # if ! aws pinpoint-sms-voice-v2 describe-pools --pool-ids $poolId > /dev/null 2>&1; then | |||
There was a problem hiding this comment.
Remove commented-out code if it is no longer needed. Keeping commented-out code can clutter the script and reduce readability.
| --pool-id $poolId \ | ||
| --origination-identity $number \ | ||
| --iso-country-code CA | ||
| echo "Requested $number" |
There was a problem hiding this comment.
Consider adding a check to ensure that the phone number request was successful before printing the requested number. This can help in identifying issues early if the request fails.
There was a problem hiding this comment.
yeah that's a good idea.
Summary | Résumé
Need to tweak the
request_long_codes.shscript because on staging and production we have custom keywords for the pools. :/Fixing this simplifies the code substantially though 🎉
Also cleaned up the drain script slightly and incorporated some AI suggestions! 🤖
Related Issues | Cartes liées
Before merging this PR
Read code suggestions left by the
cds-ai-codereviewer bot. Address
valid suggestions and shortly write down reasons to not address others. To help
with the classification of the comments, please use these reactions on each of the
comments made by the AI review:
The classifications will be extracted and summarized into an analysis of how helpful
or not the AI code review really is.
Test instructions | Instructions pour tester la modification
Add some numbers to staging and production.
(Note: currently we are at the limit of numbers on staging so the script will fail. Delete a few and then try)
Release Instructions | Instructions pour le déploiement
None.
Reviewer checklist | Liste de vérification du réviseur