-
Notifications
You must be signed in to change notification settings - Fork 369
Confirmation before push with info of branch and repo user is on #45
base: master
Are you sure you want to change the base?
Conversation
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.
Fix always aborts the run.
We don't need the extra check for protected branches.
Thank you for the feedback I have fixed the issues that you have mentioned. |
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 submitting the updates. I still think it requires a couple more comments.
Also, please include information on the PR on how you tested?
Thanks.
setup.sh
Outdated
then | ||
git push | ||
else | ||
continue |
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.
This should exit. If the program continues it will end up with a cluster running with a possibly invalid configuration.
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.
Changed it to abort only if the default is entered.
setup.sh
Outdated
echo 'Working on Repository: '$ORIGN | ||
echo 'On Branch: '$BRANCH | ||
|
||
read -p "Are you sure you want to push to \"$BRANCH\" ? (y/n): " -n 1 -r |
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.
Typically when prompting a user, you uppercase the default. Please upper case the N
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.
I did not know about this naming convention were you have to uppercase the user's default option, thank you.
At the beginning I have written the code as a sand alone script and tested the functionality on an already existing repo. When the functionality of the code worked I have then implemented the code to the setup.sh |
Regarding issue #34