Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

Confirmation before push with info of branch and repo user is on #45

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Confirmation before push with info of branch and repo user is on #45

wants to merge 5 commits into from

Conversation

dominikS007
Copy link

@dominikS007 dominikS007 commented Sep 26, 2019

Regarding issue #34

Copy link
Contributor

@palemtnrider palemtnrider left a 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.

setup.sh Outdated Show resolved Hide resolved
setup.sh Outdated Show resolved Hide resolved
setup.sh Outdated Show resolved Hide resolved
setup.sh Outdated Show resolved Hide resolved
setup.sh Outdated Show resolved Hide resolved
@dominikS007
Copy link
Author

Thank you for the feedback I have fixed the issues that you have mentioned.

Copy link
Contributor

@palemtnrider palemtnrider 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 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
Copy link
Contributor

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.

Copy link
Author

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
Copy link
Contributor

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

Copy link
Author

@dominikS007 dominikS007 Oct 7, 2019

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.

@dominikS007
Copy link
Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants