[wip]OTA-1548: set up accepted risks #2170
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,217 @@ | ||
| package accept | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "github.com/google/go-cmp/cmp" | ||
| "sort" | ||
| "strings" | ||
|
|
||
| configv1 "github.com/openshift/api/config/v1" | ||
| configv1client "github.com/openshift/client-go/config/clientset/versioned" | ||
| "github.com/spf13/cobra" | ||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| "k8s.io/apimachinery/pkg/util/sets" | ||
| "k8s.io/cli-runtime/pkg/genericiooptions" | ||
| kcmdutil "k8s.io/kubectl/pkg/cmd/util" | ||
| "k8s.io/kubectl/pkg/util/templates" | ||
| ) | ||
|
|
||
| func newOptions(streams genericiooptions.IOStreams) *options { | ||
| return &options{ | ||
| IOStreams: streams, | ||
| } | ||
| } | ||
|
|
||
| var ( | ||
| acceptExample = templates.Examples(` | ||
| # Accept RiskA and RiskB and stop accepting RiskC if accepted | ||
| oc adm upgrade accept RiskA,RiskB,-RiskC | ||
|
|
||
| # Accept RiskA and RiskB and nothing else | ||
| oc adm upgrade accept --replace RiskA,RiskB | ||
|
|
||
| # Accept no risks | ||
| oc adm upgrade accept --clear | ||
| `) | ||
|
|
||
| acceptLong = templates.LongDesc(` | ||
| Accept risks exposed to conditional updates. | ||
|
|
||
| Multiple risks are concatenated with comma. Append the provided accepted risks into the existing | ||
|
Member
There was a problem hiding this comment. Maybe "Append ..." -> "By default, the command appends..." or something that makes the actor and context for that result more clear? |
||
| list. If --replace is specified, the existing accepted risks will be replaced with the provided | ||
| ones instead of appending by default. Placing "-" as prefix to an accepted risk will lead to | ||
|
Member
There was a problem hiding this comment. nit: I'd drop "by default" here. That's context I'd rather cover in the previous sentance, where appending is the main focus, and if we cover it there, folks should still remember that appending was the default by the time they get to the end of this |
||
| removal if it exists and no-ops otherwise. If --replace is specified, the prefix "-" on the risks | ||
| is not allowed. | ||
|
|
||
| The existing accepted risks can be removed by passing --clear. | ||
|
Member
There was a problem hiding this comment. nit: Maybe "Passing --clear removes all existing excepted risks." to be extra precise on the completeness of the removal?
Member
There was a problem hiding this comment. hah, and the wording I floated matches what you have in the |
||
| `) | ||
| ) | ||
|
|
||
| func New(f kcmdutil.Factory, streams genericiooptions.IOStreams) *cobra.Command { | ||
| o := newOptions(streams) | ||
| cmd := &cobra.Command{ | ||
| Use: "accept", | ||
| Hidden: true, | ||
| Short: "Accept risks exposed to conditional updates.", | ||
| Long: acceptLong, | ||
| Example: acceptExample, | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| kcmdutil.CheckErr(o.Complete(f, cmd, args)) | ||
| kcmdutil.CheckErr(o.Run(cmd.Context())) | ||
| }, | ||
| } | ||
|
|
||
| flags := cmd.Flags() | ||
| flags.BoolVar(&o.replace, "replace", false, "Replace existing accepted risks with new ones") | ||
| flags.BoolVar(&o.clear, "clear", false, "Remove all existing accepted risks") | ||
| return cmd | ||
| } | ||
|
|
||
| // clusterVersionInterface is the subset of configv1client.ClusterVersionInterface | ||
| // that we need, for easier mocking in unit tests. | ||
| type clusterVersionInterface interface { | ||
| Get(ctx context.Context, name string, opts metav1.GetOptions) (*configv1.ClusterVersion, error) | ||
| Patch(ctx context.Context, name string, pt types.PatchType, data []byte, opts metav1.PatchOptions, subresources ...string) (result *configv1.ClusterVersion, err error) | ||
| } | ||
|
|
||
| type options struct { | ||
| genericiooptions.IOStreams | ||
|
|
||
| Client configv1client.Interface | ||
| replace bool | ||
| clear bool | ||
| plus sets.Set[string] | ||
| minus sets.Set[string] | ||
|
Member
There was a problem hiding this comment. nit: |
||
| } | ||
|
|
||
| func (o *options) Complete(f kcmdutil.Factory, cmd *cobra.Command, args []string) error { | ||
| if o.clear && o.replace { | ||
| return kcmdutil.UsageErrorf(cmd, "--clear and --replace are mutually exclusive") | ||
| } | ||
|
|
||
| if o.clear { | ||
| kcmdutil.RequireNoArguments(cmd, args) | ||
| } else if len(args) == 0 { | ||
| return kcmdutil.UsageErrorf(cmd, "no positional arguments given") | ||
| } | ||
|
|
||
| if len(args) > 1 { | ||
| return kcmdutil.UsageErrorf(cmd, "multiple positional arguments given") | ||
| } else if len(args) == 1 { | ||
| o.plus = sets.New[string]() | ||
| o.minus = sets.New[string]() | ||
| for _, s := range strings.Split(args[0], ",") { | ||
| trimmed := strings.TrimSpace(s) | ||
| if trimmed == "-" || trimmed == "" { | ||
| return kcmdutil.UsageErrorf(cmd, "illegal risk %q", trimmed) | ||
| } | ||
| if strings.HasPrefix(trimmed, "-") { | ||
| o.minus.Insert(trimmed[1:]) | ||
| } else { | ||
| o.plus.Insert(trimmed) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if conflict := o.plus.Intersection(o.minus); conflict.Len() > 0 { | ||
| return kcmdutil.UsageErrorf(cmd, "found conflicting risks: %s", strings.Join(sets.List(conflict), ",")) | ||
|
Member
There was a problem hiding this comment. nit: maybe spell out the conflict more explicitly, e.g. |
||
| } | ||
|
|
||
| if o.replace && o.minus.Len() > 0 { | ||
| return kcmdutil.UsageErrorf(cmd, "The prefix '-' on risks is not allowed if --replace is specified") | ||
| } | ||
|
|
||
| cfg, err := f.ToRESTConfig() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| client, err := configv1client.NewForConfig(cfg) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| o.Client = client | ||
| return nil | ||
| } | ||
|
|
||
| func (o *options) Run(ctx context.Context) error { | ||
| cv, err := o.Client.ConfigV1().ClusterVersions().Get(ctx, "version", metav1.GetOptions{}) | ||
| if err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| return fmt.Errorf("no cluster version information available - you must be connected to an OpenShift version 4 server to fetch the current version") | ||
|
Member
There was a problem hiding this comment. I see you're pattern-matching vs. some sibling subcommands. Mind adding a new commit to this pull to drop the |
||
| } | ||
| return err | ||
| } | ||
|
|
||
| existing := map[string]configv1.AcceptRisk{} | ||
| if cv.Spec.DesiredUpdate != nil { | ||
| for _, risk := range cv.Spec.DesiredUpdate.AcceptRisks { | ||
| existing[risk.Name] = risk | ||
|
Member
There was a problem hiding this comment. appending to a map is going to lose the in-cluster order. I think we should just pass an |
||
| } | ||
| } | ||
| acceptRisks := getAcceptRisks(existing, o.replace, o.clear, o.plus, o.minus) | ||
|
|
||
| var update *configv1.Update | ||
| if cv.Spec.DesiredUpdate != nil { | ||
| update = cv.Spec.DesiredUpdate.DeepCopy() | ||
| update.AcceptRisks = acceptRisks | ||
| } else if len(acceptRisks) > 0 { | ||
| update = &configv1.Update{ | ||
| Architecture: cv.Status.Desired.Architecture, | ||
|
Member
There was a problem hiding this comment. nit: I don't think we want to set this. The
Member
There was a problem hiding this comment. Ah, also, I guess cannot set both Architecture and Image. |
||
| Image: cv.Status.Desired.Image, | ||
| Version: cv.Status.Desired.Version, | ||
|
Member
There was a problem hiding this comment.
|
||
| AcceptRisks: acceptRisks, | ||
| } | ||
| } | ||
| if diff := cmp.Diff(update, cv.Spec.DesiredUpdate); diff != "" { | ||
| cv.Spec.DesiredUpdate = update | ||
| cv, err = o.Client.ConfigV1().ClusterVersions().Update(ctx, cv, metav1.UpdateOptions{}) | ||
|
Member
There was a problem hiding this comment. Can we please use |
||
| if err != nil { | ||
| return fmt.Errorf("unable to upgrade: %w", err) | ||
| } | ||
| var names []string | ||
| if cv.Spec.DesiredUpdate != nil { | ||
| for _, risk := range cv.Spec.DesiredUpdate.AcceptRisks { | ||
| names = append(names, risk.Name) | ||
| } | ||
| } | ||
| _, _ = fmt.Fprintf(o.Out, "info: Accept risks are [%s]\n", strings.Join(names, ", ")) | ||
| } else { | ||
| _, _ = fmt.Fprintf(o.Out, "info: Accept risks are not changed\n") | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func getAcceptRisks(existing map[string]configv1.AcceptRisk, replace, clear bool, plus sets.Set[string], minus sets.Set[string]) []configv1.AcceptRisk { | ||
| var acceptRisks []configv1.AcceptRisk | ||
|
|
||
| if clear { | ||
| return acceptRisks | ||
| } | ||
|
|
||
| for name := range plus { | ||
| if r, ok := existing[name]; ok { | ||
| acceptRisks = append(acceptRisks, r) | ||
| } else { | ||
| acceptRisks = append(acceptRisks, configv1.AcceptRisk{ | ||
| Name: name, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| if !replace { | ||
| for name, r := range existing { | ||
| if !plus.Has(name) && !minus.Has(name) { | ||
| acceptRisks = append(acceptRisks, r) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| sort.Slice(acceptRisks, func(i, j int) bool { | ||
| return acceptRisks[i].Name < acceptRisks[j].Name | ||
| }) | ||
| return acceptRisks | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| package accept | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/google/go-cmp/cmp" | ||
| configv1 "github.com/openshift/api/config/v1" | ||
| "k8s.io/apimachinery/pkg/util/sets" | ||
| ) | ||
|
|
||
| func Test_getAcceptRisks(t *testing.T) { | ||
| for _, testCase := range []struct { | ||
| name string | ||
| existing map[string]configv1.AcceptRisk | ||
| replace bool | ||
| clear bool | ||
| plus sets.Set[string] | ||
| minus sets.Set[string] | ||
| expected []configv1.AcceptRisk | ||
| }{ | ||
| { | ||
| name: "all zeros", | ||
| }, | ||
| { | ||
| name: "riskA, riskB + riskB + riskC - riskA - riskD", | ||
| existing: map[string]configv1.AcceptRisk{ | ||
| "riskA": {Name: "riskA"}, | ||
| "riskB": {Name: "riskB"}, | ||
| }, | ||
| plus: sets.New[string]("riskB", "riskC"), | ||
| minus: sets.New[string]("riskA", "riskD"), | ||
| expected: []configv1.AcceptRisk{ | ||
| {Name: "riskB"}, | ||
| {Name: "riskC"}, | ||
| }, | ||
| }, | ||
| { | ||
| name: "replace", | ||
| existing: map[string]configv1.AcceptRisk{ | ||
| "riskA": {Name: "riskA"}, | ||
| "riskB": {Name: "riskB"}, | ||
| }, | ||
| plus: sets.New[string]("riskB", "riskC"), | ||
| minus: sets.New[string]("does not matter"), | ||
| replace: true, | ||
| expected: []configv1.AcceptRisk{ | ||
| {Name: "riskB"}, | ||
| {Name: "riskC"}, | ||
| }, | ||
| }, | ||
| { | ||
| name: "clear", | ||
| existing: map[string]configv1.AcceptRisk{ | ||
| "riskA": {Name: "riskA"}, | ||
| "riskB": {Name: "riskB"}, | ||
| }, | ||
| plus: sets.New[string]("not important"), | ||
| minus: sets.New[string]("does not matter"), | ||
| clear: true, | ||
| }, | ||
| } { | ||
| t.Run(testCase.name, func(t *testing.T) { | ||
| actual := getAcceptRisks(testCase.existing, testCase.replace, testCase.clear, testCase.plus, testCase.minus) | ||
| if diff := cmp.Diff(actual, testCase.expected); diff != "" { | ||
| t.Errorf("getAcceptRisks() mismatch (-want +got):\n%s", diff) | ||
| } | ||
| }) | ||
| } | ||
| } |
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.
nit: "Manage update risk acceptance." or something that gives enough room for both acceptance and
-RiskC/--clearrejection.