bugfixes for editing managed service #763

renan-campos · 2022-06-28T11:16:30Z

Parameters not defined during service creation can now be updated.
Command now fails if invalid parameters are passed.

renan-campos · 2022-06-28T11:20:23Z

@tbrisker PTAL: A flag checking object was to the Rosa runtime object.
@ciaranRoche PTAL: That flag checking logic that was in the create service command has been generalized.

tbrisker

Not sure how I feel adding argument related logic to the runtime, perhaps we should keep a separation between them?

pkg/rosa/runtime.go

tbrisker · 2022-06-28T11:28:37Z

cmd/edit/service/cmd.go

 	"github.com/openshift/rosa/pkg/arguments"
 	"github.com/openshift/rosa/pkg/ocm"
 	"github.com/openshift/rosa/pkg/rosa"
 )

 var args ocm.UpdateManagedServiceArgs

+var runtime *rosa.Runtime


I'm not too fond of using a global for the runtime (or in general)

Me neither, I thought I couldn't do it without using a global variable but I just figured out a way.

Please resolve this if the removal of the global variable works for you.

pkg/arguments/validator.go

renan-campos · 2022-06-28T15:13:53Z

Not sure how I feel adding argument related logic to the runtime, perhaps we should keep a separation between them?

@tbrisker I can change it if needed, but how should we determine what goes in the runtime object? I thought the FlagChecker fit in with the other objects in the Runtime: objects that will be used during the run.

tbrisker · 2022-06-29T06:07:18Z

@renan-campos I think the runtime should be limited to what happens in the run() stage of the command. That also prevents the need for the global variable. Perhaps we can add another similar object that can be used in the init() stage and handles flags, args etc. Maybe in the future we could even move some of the validations that currently run in run() to init(), but we'll need to dig a bit deeper into cobra and pflags to figure that out better.

renan-campos · 2022-06-29T12:53:01Z

@renan-campos I think the runtime should be limited to what happens in the run() stage of the command. That also prevents the need for the global variable. Perhaps we can add another similar object that can be used in the init() stage and handles flags, args etc. Maybe in the future we could even move some of the validations that currently run in run() to init(), but we'll need to dig a bit deeper into cobra and pflags to figure that out better.

That makes sense; Last time you reviewed the code the checker had to be globally scoped, but I changed it so that all of it occurs in the run stage. With that being the case, does it now belong in runtime, or would it still be better as a separate object?

tbrisker · 2022-06-29T13:15:33Z

cmd/edit/service/cmd.go

-
-	if len(parameters) == 0 {
-		r.Reporter.Errorf("Service %q has no parameters to edit", args.ID)
+	// Setting parameter flags as valid


this seems out of place?

tbrisker · 2022-06-29T13:31:38Z

cmd/edit/service/cmd.go

+	// Adding known flags to flag checker before parsing the unknown flags
+	cmd.Flags().VisitAll(func(flag *pflag.Flag) {
+		r.FlagChecker.AddValidFlag(flag)
+	})
+
+	err := arguments.ParseUnknownFlags(cmd, argv)
+	if err != nil {
+		r.Reporter.Errorf("Failed to parse flags: %v", err)
+		os.Exit(1)
+	}
+
+	if len(cmd.Flags().Args()) > 0 {
+		r.Reporter.Errorf("Unrecognized command line parameter")
+		os.Exit(1)
+	}
+


This looks pretty generic, maybe that be done inside WithFlagChecker()? perhaps pass cmd to the NewRuntime() method as that might be useful in other places for refactoring, or into WithFlagChecker()?

Still not sure if this should be in the runtime itself or the FlagCheck object... Perhaps lets start with the separate object and we can move it into the runtime if we see this becomes a common pattern?

tbrisker · 2022-06-29T14:09:00Z

cmd/edit/service/cmd.go

+	cmd.Flags().VisitAll(func(flag *pflag.Flag) {
+		if !r.FlagChecker.IsValidFlag(flag) {
+			r.Reporter.Errorf("%q is not a valid flag", flag.Name)
+			os.Exit(1)
+		}
+	})


This is also generic and perhaps shouldn't be here - if the FlagChecker has access to cmd (whether directly or via runtime) this could become something like FlagChecker.ValidateFlags()

tbrisker · 2022-06-29T14:43:16Z

pkg/arguments/validator.go

+	return f
+}
+
+func (f *FlagCheck) AddValidParameter(parameterName string) *FlagCheck {


this seems very addons-specific, perhaps we could combine this with AddValidFlag and have one method that accepts a string and leave it to the caller to pass the right string?

tbrisker · 2022-06-29T14:53:51Z

cmd/edit/service/cmd.go

+		r.FlagChecker.AddValidFlag(flag)
+	})
+
+	err := arguments.ParseUnknownFlags(cmd, argv)


Just a thought, lmk what you think:
This will add all the unknown flags to cmd.Flags() to check if they are valid later.
i'm wondering if we could perhaps flip the order around, add the flags from the addon params, and then get rid of keeping a separate set of valid flags and instead use cmd.Flags().Lookup(..) to check if a flag is valid or not?

The barrier to this approach is that the flags need to be parsed before the addon parameters are known.
So the flow needs to go:

Parse flags

Get the addon related to the service id

Ensure the user provided flags are original command flags or add-on parameter flags.

Having to use some of the flags before being able to check all the flags is what really throws me off trying to get a good flow here.

ParseUnknownFlags mutating the cmdFlagset doesn't help...

I'm not sure, but i think you should be able to access the known flags (which is what you need to get the other ones) even before parsing the unknown ones

Ok I tried it that way.

renan-campos · 2022-06-29T16:37:52Z

It's coming together...

tbrisker · 2022-06-30T11:14:46Z

pkg/rosa/runtime.go

+	OCMClient   *ocm.Client
+	AWSClient   aws.Client
+	Creator     *aws.Creator
+	FlagChecker *arguments.FlagCheck


This isn't needed for now

tbrisker · 2022-06-30T11:51:08Z

cmd/edit/service/cmd.go

+		r.FlagChecker.AddValidFlag(flag)
+	})
+
+	err := arguments.ParseUnknownFlags(cmd, argv)


I'm not sure, but i think you should be able to access the known flags (which is what you need to get the other ones) even before parsing the unknown ones

pkg/arguments/arguments.go

ciaranRoche · 2022-07-05T15:55:49Z

Lets merge this in, we can follow up with any other changes @tbrisker wants when he returns.

ciaranRoche · 2022-07-05T15:55:55Z

/lgtm

openshift-ci · 2022-07-05T15:58:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ciaranRoche, renan-campos

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~~ [ciaranRoche]

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

openshift-ci · 2022-07-05T15:58:52Z

@renan-campos: 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-ci bot requested review from igoihman and vkareh June 28, 2022 11:17

renan-campos changed the title ~~edit service can update parameters that weren't originally defined.~~ edit service bugfixes Jun 28, 2022

renan-campos changed the title ~~edit service bugfixes~~ bugfixes for editing managed service Jun 28, 2022

tbrisker reviewed Jun 28, 2022

View reviewed changes

renan-campos force-pushed the ms_edit branch from ac560b7 to dc84077 Compare June 28, 2022 14:29

renan-campos force-pushed the ms_edit branch from dc84077 to d7e73ea Compare June 29, 2022 01:04

edit service can update parameters that weren't originally defined.

d7e73ea

tbrisker reviewed Jun 29, 2022

View reviewed changes

renan-campos closed this Jun 29, 2022

renan-campos reopened this Jun 29, 2022

addressing pr comments

ca547e1

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2022

tbrisker reviewed Jun 30, 2022

View reviewed changes

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2022

renan-campos force-pushed the ms_edit branch from c021141 to 2f9ac70 Compare July 1, 2022 14:44

A different approach to parsing the flags.

2f9ac70

ciaranRoche reviewed Jul 5, 2022

View reviewed changes

pkg/arguments/arguments.go Outdated Show resolved Hide resolved

renan-campos force-pushed the ms_edit branch from 5c93305 to a4d977c Compare July 5, 2022 15:49

removed validator object

a4d977c

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

openshift-ci bot assigned ciaranRoche Jul 5, 2022

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

openshift-ci bot merged commit af324f9 into openshift:master Jul 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfixes for editing managed service #763

bugfixes for editing managed service #763

renan-campos commented Jun 28, 2022 •

edited

Loading

renan-campos commented Jun 28, 2022

tbrisker left a comment

tbrisker Jun 28, 2022

renan-campos Jun 29, 2022

renan-campos Jun 29, 2022

renan-campos commented Jun 28, 2022 •

edited

Loading

tbrisker commented Jun 29, 2022

renan-campos commented Jun 29, 2022 •

edited

Loading

tbrisker Jun 29, 2022

tbrisker Jun 29, 2022

tbrisker Jun 29, 2022

tbrisker Jun 29, 2022

tbrisker Jun 29, 2022

renan-campos Jun 29, 2022

renan-campos Jun 29, 2022

renan-campos Jun 29, 2022

tbrisker Jun 30, 2022

renan-campos Jun 30, 2022

renan-campos commented Jun 29, 2022

tbrisker Jun 30, 2022

tbrisker Jun 30, 2022

ciaranRoche commented Jul 5, 2022

ciaranRoche commented Jul 5, 2022

openshift-ci bot commented Jul 5, 2022

openshift-ci bot commented Jul 5, 2022

bugfixes for editing managed service #763

bugfixes for editing managed service #763

Conversation

renan-campos commented Jun 28, 2022 • edited Loading

renan-campos commented Jun 28, 2022

tbrisker 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

renan-campos commented Jun 28, 2022 • edited Loading

tbrisker commented Jun 29, 2022

renan-campos commented Jun 29, 2022 • edited Loading

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

renan-campos commented Jun 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciaranRoche commented Jul 5, 2022

ciaranRoche commented Jul 5, 2022

openshift-ci bot commented Jul 5, 2022

openshift-ci bot commented Jul 5, 2022

renan-campos commented Jun 28, 2022 •

edited

Loading

renan-campos commented Jun 28, 2022 •

edited

Loading

renan-campos commented Jun 29, 2022 •

edited

Loading