WINC-949: Rename powershellVariablesInCommand #1352

sebsoto · 2022-12-05T18:51:26Z

Renames powershellVariablesInCommand to powershellPreScripts to remove the implicit requirement of a variable to replace, which is not always necessary. Also renames powershellVaraiblesInCommand.Name, to VariableName, to be more clear about the purpose of the field with the new naming.

openshift-ci · 2022-12-05T18:51:30Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

sebsoto · 2022-12-05T18:51:56Z

/test build

sebsoto · 2022-12-05T18:52:06Z

/test lint

sebsoto · 2022-12-05T18:52:22Z

/approve cancel

saifshaikh48

/lgtm

thanks for doing this ⚡

aravindhp

Thanks for working on this, @sebsoto. Please address my comments.

aravindhp · 2022-12-05T19:27:59Z

pkg/services/services.go

+		Priority:               1,
+		Bootstrap:              true,
+		Dependencies:           []string{windows.ContainerdServiceName},
+		PowershellPreScripts:   powershellVars,


Now this looks a bit odd. Why would PowershellPreScripts be referred to as powershellVars

aravindhp · 2022-12-05T19:29:28Z

pkg/daemon/controller/controller.go

 		}
-		if psVar.Name != "" {
-			vars[psVar.Name] = strings.TrimSpace(out)
+		if psVar.VariableName != "" {


Now this variable is stuttering because this is no longer a psVar.

aravindhp · 2022-12-05T19:31:19Z

pkg/servicescm/servicescm.go

-	Name string `json:"name"`
-	// Path is the location of a PowerShell script whose output is the value of the variable
+// PowershellPreScript describes a PowerShell script to be ran and an optional variable to be populated
+type PowershellPreScript struct {


If you are thinking about PowerShellPostScript shouldn't this be generic?

PowerShellPostScript would just need to be a []string, as there wouldn't be any variables to replace, given it runs after the service.

Would this also be the case for arbitrary scripts that we were going to introduce instead of always having scripts associated with a service? While there might not be variables to replace there either do you think the struct could be reused with other members added to it?

aravindhp · 2022-12-05T20:48:41Z

pkg/servicescm/servicescm.go

+type PowershellPreScript struct {
+	// VariableName is the name of a variable which should be replaced by the output of the script. An empty value will
+	// cause no variable replacement to occur, but the script will still be ran.
+	VariableName string `json:"variableName"`


Do we need to add omitempty?

Yes, I think so. In addition, would be nice to add a unit test to cover this scenario, where a PowershellPreScript gets created without VariableName.

openshift-ci · 2022-12-05T20:49:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aravindhp

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [aravindhp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jrvaldes

LGTM, just a few suggestions.

jrvaldes · 2022-12-05T21:00:25Z

pkg/services/services.go

@@ -67,9 +67,7 @@ func containerdConfiguration(debug bool) servicescm.Service {
 		Name:                   windows.ContainerdServiceName,
 		Command:                containerdServiceCmd,
 		NodeVariablesInCommand: nil,
-		PowershellVariablesInCommand: []servicescm.PowershellCmdArg{{
-			// Name is left blank as we just want this script to execute, without any service command replacements


It may be helpful to keep the comment.

jrvaldes · 2022-12-05T21:06:41Z

pkg/servicescm/servicescm.go

+type PowershellPreScript struct {
+	// VariableName is the name of a variable which should be replaced by the output of the script. An empty value will
+	// cause no variable replacement to occur, but the script will still be ran.
+	VariableName string `json:"variableName"`


Yes, I think so. In addition, would be nice to add a unit test to cover this scenario, where a PowershellPreScript gets created without VariableName.

saifshaikh48

/lgtm

LGTM from my end

Renames powershellVariablesInCommand to powershellPreScripts to remove the implicit requirement of a variable to replace, which is not always necessary. Also renames powershellVaraiblesInCommand.Name, to VariableName, to be more clear about the purpose of the field with the new naming.

sebsoto · 2022-12-06T14:04:36Z

/test aws-e2e-operator

sebsoto · 2022-12-06T14:04:41Z

/test gcp-e2e-operator

jrvaldes · 2022-12-06T14:38:33Z

/lgtm

sebsoto · 2022-12-06T16:44:59Z

/retest-required

openshift-ci-robot · 2022-12-06T18:34:00Z

/retest-required

Remaining retests: 0 against base HEAD 1b76b08 and 2 for PR HEAD c4ec11c in total

sebsoto · 2022-12-06T19:03:01Z

/retest-required

sebsoto · 2022-12-06T19:58:24Z

/cherry-pick release-4.12

openshift-cherrypick-robot · 2022-12-06T19:58:26Z

@sebsoto: once the present PR merges, I will cherry-pick it on top of release-4.12 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.12

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2022-12-06T21:17:38Z

@sebsoto: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-cherrypick-robot · 2022-12-06T21:21:17Z

@sebsoto: #1352 failed to apply on top of branch "release-4.12":

Applying: Rename powershellVariablesInCommand
Using index info to reconstruct a base tree...
M	pkg/daemon/controller/controller.go
M	pkg/daemon/controller/controller_test.go
M	pkg/services/services.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/services/services.go
CONFLICT (content): Merge conflict in pkg/services/services.go
Auto-merging pkg/daemon/controller/controller_test.go
CONFLICT (content): Merge conflict in pkg/daemon/controller/controller_test.go
Auto-merging pkg/daemon/controller/controller.go
CONFLICT (content): Merge conflict in pkg/daemon/controller/controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Rename powershellVariablesInCommand
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.12

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mansikulkarni96 · 2023-02-28T03:51:22Z

/cherry-pick release-4.12

openshift-cherrypick-robot · 2023-02-28T03:52:00Z

@mansikulkarni96: new pull request created: #1436

In response to this:

/cherry-pick release-4.12

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 5, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2022

openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2022

saifshaikh48 reviewed Dec 5, 2022

View reviewed changes

openshift-ci bot assigned saifshaikh48 Dec 5, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2022

aravindhp suggested changes Dec 5, 2022

View reviewed changes

sebsoto force-pushed the renameCMField branch from 994b1ca to f630791 Compare December 5, 2022 20:19

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2022

aravindhp reviewed Dec 5, 2022

View reviewed changes

aravindhp approved these changes Dec 5, 2022

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2022

jrvaldes reviewed Dec 5, 2022

View reviewed changes

saifshaikh48 reviewed Dec 5, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2022

sebsoto force-pushed the renameCMField branch from f630791 to c4ec11c Compare December 6, 2022 14:04

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2022

openshift-ci bot assigned jrvaldes Dec 6, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2022

sebsoto marked this pull request as ready for review December 6, 2022 16:43

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2022

openshift-ci bot requested review from aravindhp and mansikulkarni96 December 6, 2022 16:49

sebsoto mentioned this pull request Dec 6, 2022

[release-4.12] WINC-949: Rename powershellVariablesInCommand #1356

Merged

openshift-merge-robot merged commit 505d39f into openshift:master Dec 6, 2022

openshift-cherrypick-robot mentioned this pull request Feb 28, 2023

[release-4.12] WINC-949: Rename powershellVariablesInCommand #1436

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WINC-949: Rename powershellVariablesInCommand #1352

WINC-949: Rename powershellVariablesInCommand #1352

sebsoto commented Dec 5, 2022

openshift-ci bot commented Dec 5, 2022

sebsoto commented Dec 5, 2022

sebsoto commented Dec 5, 2022

sebsoto commented Dec 5, 2022

saifshaikh48 left a comment

aravindhp left a comment

aravindhp Dec 5, 2022

aravindhp Dec 5, 2022

aravindhp Dec 5, 2022

sebsoto Dec 5, 2022

aravindhp Dec 5, 2022

aravindhp Dec 5, 2022

jrvaldes Dec 5, 2022

openshift-ci bot commented Dec 5, 2022

jrvaldes left a comment •

edited

jrvaldes Dec 5, 2022

jrvaldes Dec 5, 2022

saifshaikh48 left a comment

sebsoto commented Dec 6, 2022

sebsoto commented Dec 6, 2022

jrvaldes commented Dec 6, 2022

sebsoto commented Dec 6, 2022

openshift-ci-robot commented Dec 6, 2022

sebsoto commented Dec 6, 2022

sebsoto commented Dec 6, 2022

openshift-cherrypick-robot commented Dec 6, 2022

openshift-ci bot commented Dec 6, 2022

openshift-cherrypick-robot commented Dec 6, 2022

mansikulkarni96 commented Feb 28, 2023

openshift-cherrypick-robot commented Feb 28, 2023

WINC-949: Rename powershellVariablesInCommand #1352

WINC-949: Rename powershellVariablesInCommand #1352

Conversation

sebsoto commented Dec 5, 2022

openshift-ci bot commented Dec 5, 2022

sebsoto commented Dec 5, 2022

sebsoto commented Dec 5, 2022

sebsoto commented Dec 5, 2022

saifshaikh48 left a comment

Choose a reason for hiding this comment

aravindhp left a 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

openshift-ci bot commented Dec 5, 2022

jrvaldes left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saifshaikh48 left a comment

Choose a reason for hiding this comment

sebsoto commented Dec 6, 2022

sebsoto commented Dec 6, 2022

jrvaldes commented Dec 6, 2022

sebsoto commented Dec 6, 2022

openshift-ci-robot commented Dec 6, 2022

sebsoto commented Dec 6, 2022

sebsoto commented Dec 6, 2022

openshift-cherrypick-robot commented Dec 6, 2022

openshift-ci bot commented Dec 6, 2022

openshift-cherrypick-robot commented Dec 6, 2022

mansikulkarni96 commented Feb 28, 2023

openshift-cherrypick-robot commented Feb 28, 2023

jrvaldes left a comment •

edited