-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
K8SPG-391 openshift certification #556
Conversation
…a/percona-postgresql-operator into K8SPG-391_openshift_certification
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…a/percona-postgresql-operator into K8SPG-391_openshift_certification
e2e-tests/functions
Outdated
create_namespace() { | ||
local namespace=$1 | ||
if [[ ${CLEAN_NAMESPACE} == 1 ]]; then | ||
if [ -n "$OPENSHIFT" ]; then |
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.
Double square brackets [[ ]] are generally preferable over single square brackets [ ] in Bash. Would you consider, for example, using
if [[ $OPENSHIFT ]]; then
here?
Also, the quotes around "$OPENSHIFT"
are superfluous.
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.
O! Nice! I will fix it tomorrow.
e2e-tests/functions
Outdated
@@ -5,14 +5,33 @@ ROOT_REPO=${ROOT_REPO:-$(realpath ../../..)} | |||
source "${ROOT_REPO}/e2e-tests/vars.sh" | |||
test_name=$(basename "$(pwd)") | |||
|
|||
if oc get projects; then |
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.
did you mean
if oc get projects > /dev/null 2>&1; then
e2e-tests/functions
Outdated
create_namespace() { | ||
local namespace=$1 | ||
if [[ ${CLEAN_NAMESPACE} == 1 ]]; then | ||
if [ -n "$OPENSHIFT" ]; then | ||
if [ -n "$OPERATOR_NS" -a $(oc get project "$OPERATOR_NS" -o json | jq -r '.metadata.name') ]; then |
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.
- same as above for the double square brackets
- also, are you sure this is the comparison you actually mean? Did you mean:
set -o pipefail
if [[ "$OPERATOR_NS" ]] && oc get project "$OPERATOR_S" -o json > /dev/null 2>&1 | jq -r '.metadata.name' > /dev/null 2>&1; then
e2e-tests/functions
Outdated
@@ -240,8 +259,23 @@ wait_for_delete() { | |||
deploy_pmm_server() { | |||
local platform=kubernetes | |||
helm uninstall -n "${NAMESPACE}" pmm || : | |||
helm install monitoring -n "${NAMESPACE}" --set imageTag=${IMAGE_PMM_SERVER#*:} --set service.type="LoadBalancer" \ | |||
--set imageRepo=${IMAGE_PMM_SERVER%:*} --set platform="${platform}" "https://percona-charts.storage.googleapis.com/pmm-server-${PMM_SERVER_VERSION}.tgz" | |||
if [ ! -z "$OPENSHIFT" ]; then |
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.
same as above about [[
e2e-tests/functions
Outdated
helm install monitoring --set imageTag=${IMAGE_PMM_SERVER#*:} --set imageRepo=${IMAGE_PMM_SERVER%:*} --set platform=$platform --set sa=pmm-server --set supresshttp2=false https://percona-charts.storage.googleapis.com/pmm-server-${PMM_SERVER_VERSION}.tgz -n "${NAMESPACE}" | ||
else | ||
helm install monitoring -n "${NAMESPACE}" --set imageTag=${IMAGE_PMM_SERVER#*:} --set service.type="LoadBalancer" \ | ||
--set imageRepo=${IMAGE_PMM_SERVER%:*} --set platform="${platform}" "https://percona-charts.storage.googleapis.com/pmm-server-${PMM_SERVER_VERSION}.tgz" |
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.
in this case, what is the value of $platform
?
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.
kubernetes
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.
Where is the variable $platform
initialized as "kubernetes"? I couldn't find that.
e2e-tests/functions
Outdated
--set imageRepo=${IMAGE_PMM_SERVER%:*} --set platform="${platform}" "https://percona-charts.storage.googleapis.com/pmm-server-${PMM_SERVER_VERSION}.tgz" | ||
if [ ! -z "$OPENSHIFT" ]; then | ||
platform=openshift | ||
oc create sa pmm-server -n "${NAMESPACE}" |
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.
Generally, with some very few exceptions, there is no need to use ${VARIABLE}
in Bash. Just $VARIABLE
is enough, avoids clutter and makes the code easier to read.
e2e-tests/functions
Outdated
create_namespace() { | ||
local namespace=$1 | ||
if [[ ${CLEAN_NAMESPACE} == 1 ]]; then | ||
if [[ -n $OPENSHIFT ]]; then |
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.
-n
is not needed here, just if [[ $OPENSHIFT ]]; then
is perfectly fine
e2e-tests/functions
Outdated
create_namespace() { | ||
local namespace=$1 | ||
if [[ ${CLEAN_NAMESPACE} == 1 ]]; then | ||
if [[ -n $OPENSHIFT ]]; then | ||
if [[ -n $OPERATOR_NS && $(oc get project "$OPERATOR_NS" -o json | jq -r '.metadata.name') ]]; then |
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.
-n
is not needed here just like above, also, I think the second part of the comparrison must be outside of the square brackets, like this:
if [[ "$OPERATOR_NS" ]] && oc get project "$OPERATOR_S" -o json > /dev/null 2>&1 | jq -r '.metadata.name' > /dev/null 2>&1; then
because the command oc get projec ...
would return some string which will always be true
e2e-tests/functions
Outdated
@@ -240,8 +259,23 @@ wait_for_delete() { | |||
deploy_pmm_server() { | |||
local platform=kubernetes | |||
helm uninstall -n "${NAMESPACE}" pmm || : | |||
helm install monitoring -n "${NAMESPACE}" --set imageTag=${IMAGE_PMM_SERVER#*:} --set service.type="LoadBalancer" \ | |||
--set imageRepo=${IMAGE_PMM_SERVER%:*} --set platform="${platform}" "https://percona-charts.storage.googleapis.com/pmm-server-${PMM_SERVER_VERSION}.tgz" | |||
if [[ -n $OPENSHIFT ]]; then |
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.
no need for -n
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@nmarukovich should we set |
e2e-tests/functions
Outdated
@@ -312,6 +345,9 @@ deploy_chaos_mesh() { | |||
|
|||
helm repo add chaos-mesh https://charts.chaos-mesh.org | |||
helm install chaos-mesh chaos-mesh/chaos-mesh --namespace=${NAMESPACE} --set chaosDaemon.runtime=containerd --set chaosDaemon.socketPath=/run/containerd/containerd.sock --set dashboard.create=false --version 2.5.1 | |||
if [ ! -z "$OPENSHIFT" ]; then |
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.
We can use if [[ $OPENSHIFT ]];
like above or also if [ -n "$OPENSHIFT" ]
instead of negation.
…a/percona-postgresql-operator into K8SPG-391_openshift_certification
I can't see any difference do we use this option or not. As I understood from the code we check it automatically |
commit: f9b0017 |
CHANGE DESCRIPTION
Problem:
Short explanation of the problem.
Cause:
Short explanation of the root cause of the issue if applicable.
Solution:
Short explanation of the solution we are providing with this PR.
CHECKLIST
Jira
Needs Doc
) and QA (Needs QA
)?Tests
Config/Logging/Testability