tests: General improvements to operator script #386

GabyCT · 2024-06-17T20:53:31Z

This PR gives general improvements to the operator script that is being used in tests. This PR includes changes like better variable definition and the use of a variable that has been already defined.

fitzthum

LGTM

stevenhorsman

LGTM. Thanks @GabyCT

ldoktor

Thanks, those are fine changes although shellcheck complains about more. Could you please take care of the others too since you are already treating style issues?

GabyCT · 2024-06-25T22:47:58Z

changes applied

ldoktor · 2024-06-26T08:49:45Z

tests/e2e/operator.sh

-
-		local pod_id="$(get_pods_regex $pod $op_ns)"
+		local pod_id
+		pod_id="$(get_pods_regex $pod $op_ns)"


The $pod and $op_ns should be quoted as well

ldoktor · 2024-06-26T08:52:45Z

tests/e2e/operator.sh

-
-			local pod_id="$(get_pods_regex $pod $op_ns)"
+			local pod_id
+			pod_id="$(get_pods_regex $pod $op_ns)"


The same here

ldoktor · 2024-06-26T08:52:51Z

tests/e2e/operator.sh

 	cmd+="egrep -q ${controller_pod}.*'\<Running\>'"
 	if ! wait_for_process 120 10 "$cmd"; then
 		echo "::error:: ${controller_pod} pod is not running"

-		local pod_id="$(get_pods_regex $controller_pod $op_ns)"
+		local pod_id
+		pod_id="$(get_pods_regex $controller_pod $op_ns)"


ldoktor · 2024-06-26T08:57:37Z

tests/e2e/operator.sh

@@ -132,12 +135,12 @@ install_ccruntime() {
 	local pod=""
 	local cmd=""
 	for pod in cc-operator-daemon-install cc-operator-pre-install-daemon; do
-		cmd="kubectl get pods -n "$op_ns" --no-headers |"
+		cmd="kubectl get pods -n $op_ns --no-headers |"


and here as well cmd="kubectl get pods -n '$op_ns' --no-headers |"

ldoktor · 2024-06-26T08:57:57Z

tests/e2e/operator.sh

@@ -132,12 +135,12 @@ install_ccruntime() {
 	local pod=""
 	local cmd=""
 	for pod in cc-operator-daemon-install cc-operator-pre-install-daemon; do
-		cmd="kubectl get pods -n "$op_ns" --no-headers |"
+		cmd="kubectl get pods -n $op_ns --no-headers |"
 		cmd+="egrep -q ${pod}.*'\<Running\>'"


and here...

ldoktor · 2024-06-27T07:01:10Z

tests/e2e/operator.sh

@@ -132,12 +135,12 @@ install_ccruntime() {
 	local pod=""
 	local cmd=""
 	for pod in cc-operator-daemon-install cc-operator-pre-install-daemon; do
-		cmd="kubectl get pods -n "$op_ns" --no-headers |"
+		cmd="kubectl get pods -n '$op_ns' --no-headers |"


Thanks, this one is fine as it's a variable substitution, but ... (see below)

ldoktor · 2024-06-27T07:01:57Z

tests/e2e/operator.sh

-
-			local pod_id="$(get_pods_regex $pod $op_ns)"
+			local pod_id
+			pod_id="$(get_pods_regex '$pod' '$op_ns')"


^^ unlike variable substitution in shell execution the expansion starts over inside $( ... ) therefore here you need to use " to expand the variables.

ldoktor · 2024-06-27T07:02:12Z

tests/e2e/operator.sh

-
-		local pod_id="$(get_pods_regex $pod $op_ns)"
+		local pod_id
+		pod_id="$(get_pods_regex '$pod' '$op_ns')"


the same here, double quotes please

@ldoktor thanks for the feedback changes applied

ldoktor · 2024-07-02T04:59:04Z

tests/e2e/operator.sh

 	cmd+="egrep -q ${controller_pod}.*'\<Running\>'"
 	if ! wait_for_process 120 10 "$cmd"; then
 		echo "::error:: ${controller_pod} pod is not running"

-		local pod_id="$(get_pods_regex $controller_pod $op_ns)"
+		local pod_id
+		pod_id="$(get_pods_regex '$controller_pod' '$op_ns')"


This one still needs to be treated (" vs. ')

(It should be the last thing, consider it acked once it is addressed - I'm on PTO already, not sure when I get to it)

@ldoktor changes applied

This PR gives general improvements to the operator script that is being used in tests. This PR includes changes like better variable definition and the use of a variable that has been already defined. Signed-off-by: Gabriela Cervantes <gabriela.cervantes.tellez@intel.com>

ldoktor

Thanks, lgtm now :-) (I'm sorry for the delay, it's PTO time...)

ldoktor · 2024-07-17T05:05:30Z

@stevenhorsman @wainersm the license check failed, do you have any experience with that? (this change should not have affected that)

stevenhorsman · 2024-07-17T08:44:34Z

@stevenhorsman @wainersm the license check failed, do you have any experience with that? (this change should not have affected that)

I think we can ignore it as no-one in development has access to the report to check the issue. There seems to have been some sort of org setting that has triggered this check running on all PRs as we have the same issue in the CAA repo.

ldoktor · 2024-07-17T14:37:33Z

Well based on https://app.fossa.com/projects/git%2Bgithub.com%2Fconfidential-containers%2Foperator/refs/branch/main/abe928347a0ff9c3ecbeafceb0c761cc33cd4af1/browse/licenses I'd say it complains about go.mongodb.org/mongo-driver dependency, but that one is not new. Anyway I don't have access to the full report and also think it should be merged.

fitzthum · 2024-07-17T14:40:29Z

The license check has been failing for a while. Unfortunately it's really hard to get details on why. Nobody has an account with FOSSA, but sometimes you can stumble onto the issue on the FOSSA page. Last I checked it was actually complaining about Kata, but I'm not sure what is happening this time. I wouldn't let it block this PR.

ldoktor · 2024-07-17T14:43:59Z

Thanks for the confirmation, let me merge this awesome numbered PR :-)

fitzthum approved these changes Jun 18, 2024

View reviewed changes

stevenhorsman approved these changes Jun 18, 2024

View reviewed changes

ldoktor reviewed Jun 19, 2024

View reviewed changes

GabyCT force-pushed the topic/fixoperatorscript branch 2 times, most recently from f86a4a8 to 8891258 Compare June 25, 2024 22:45

ldoktor reviewed Jun 26, 2024

View reviewed changes

GabyCT force-pushed the topic/fixoperatorscript branch 2 times, most recently from a7b9fd5 to b4740ef Compare June 26, 2024 16:43

ldoktor reviewed Jun 27, 2024

View reviewed changes

GabyCT force-pushed the topic/fixoperatorscript branch from b4740ef to bd770b1 Compare July 1, 2024 19:09

ldoktor reviewed Jul 2, 2024

View reviewed changes

GabyCT force-pushed the topic/fixoperatorscript branch from bd770b1 to abe9283 Compare July 2, 2024 15:35

ldoktor added the ok-to-test label Jul 16, 2024

ldoktor approved these changes Jul 16, 2024

View reviewed changes

ldoktor merged commit 23aeb68 into confidential-containers:main Jul 17, 2024
13 of 14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: General improvements to operator script #386

tests: General improvements to operator script #386

GabyCT commented Jun 17, 2024

fitzthum left a comment

stevenhorsman left a comment

ldoktor left a comment

GabyCT commented Jun 25, 2024

ldoktor Jun 26, 2024

ldoktor Jun 26, 2024

ldoktor Jun 26, 2024

ldoktor Jun 26, 2024

ldoktor Jun 26, 2024

ldoktor Jun 27, 2024

ldoktor Jun 27, 2024

ldoktor Jun 27, 2024

GabyCT Jul 1, 2024

ldoktor Jul 2, 2024

ldoktor Jul 2, 2024

GabyCT Jul 2, 2024

ldoktor left a comment •

edited

Loading

ldoktor commented Jul 17, 2024

stevenhorsman commented Jul 17, 2024

ldoktor commented Jul 17, 2024

fitzthum commented Jul 17, 2024

ldoktor commented Jul 17, 2024

tests: General improvements to operator script #386

tests: General improvements to operator script #386

Conversation

GabyCT commented Jun 17, 2024

fitzthum left a comment

Choose a reason for hiding this comment

stevenhorsman left a comment

Choose a reason for hiding this comment

ldoktor left a comment

Choose a reason for hiding this comment

GabyCT commented Jun 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldoktor left a comment • edited Loading

Choose a reason for hiding this comment

ldoktor commented Jul 17, 2024

stevenhorsman commented Jul 17, 2024

ldoktor commented Jul 17, 2024

fitzthum commented Jul 17, 2024

ldoktor commented Jul 17, 2024

ldoktor left a comment •

edited

Loading