Skip to content

Conversation

@aliok
Copy link

@aliok aliok commented Feb 7, 2020

Some improvements. Comments inline

Vincent Hou and others added 2 commits February 5, 2020 11:40
This PR adds the tests to verify the correct number and names of knative eventing
deployments.

The test tag postupgrade is added, marking the tests to run after upgrade to the
latest HEAD of operator, with the latest generated manifest of knative eventing.

OPERATOR_DIR=$(dirname $0)/..
# inherited from vendor/knative.dev/test-infra/scripts/library.sh
declare -A IS_PROW
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is to be used later. it is inherited from the library.sh in test-infra

# inherited from vendor/knative.dev/test-infra/scripts/library.sh
declare -A IS_PROW

OPERATOR_DIR=$(cd $(dirname "$0")/.. && pwd)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed
OPERATOR_DIR=$(dirname $0)/.. to
OPERATOR_DIR=$(cd $(dirname "$0")/.. && pwd)
.

First one results in a relative path and when we cd into eventing folder later in the script, cding back with that relative path goes to another directory relative to eventing, not eventing-operator.

Second one results in an absolute path and cding to that path always works.

Comment on lines +78 to +82
if (( IS_PROW )); then
ko resolve -P -f ${config}/ | "${LABEL_YAML_CMD[@]}" > ${yaml}
else
ko resolve -f ${config}/ | "${LABEL_YAML_CMD[@]}" > ${yaml}
fi
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use -P when the script is running local. Can't push to Docker when image paths are preserved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I switched to Quay and this problem is gone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant