-
Notifications
You must be signed in to change notification settings - Fork 27
Conversation
This PR should be merged after the corresponding PR in gp-extensions-ci is merged. |
+1. |
concourse/test.sh
Outdated
sudo passwd --delete gpadmin # for `su gpadmin` in start_gpdb | ||
echo 'alias su="sudo su"' >>"$HOME/.bashrc" | ||
source "$HOME/.bashrc" # for `alias su` | ||
start_gpdb |
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.
why not putting start_gpdb
to pre_test.sh
like others? https://github.com/pivotal/advanced-password-check/blob/gpdb/concourse/pre_test.sh
pre_test.sh
runs with root permission, no need to hack thesu
/sudo
anymore- it will add
source
to.bashrc
, so everything will be set up when gpadmin login. - Ideally, things in
pre_test.sh
do setup things, and runs only once. Sotest.sh
can be run again when hijack to the concourse.
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 totally agree that it is good to make test.sh
idempotent. Working on it.
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.
test.sh is modified to be idempotent. I tried manually and it seems to work.
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.
Any special reason why not just follow the convention to have a pre_test.sh
to start gpdb there?
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.
source "$CI_REPO_DIR/common/entry_common.sh" | ||
sudo passwd --delete gpadmin # for `su gpadmin` in start_gpdb | ||
if [[ "$(source /etc/os-release && echo "$ID")" == "ubuntu" ]]; then | ||
su () { |
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 don't like this kind of hack. 🤣
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.
There are some hacks in the script which are not necessary in my opinion. But it is OK if it works well. 😆
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.
There are some hacks in the script which are not necessary in my opinion. But it is OK if it works well. 😆
Agreed.
No description provided.