CNF-11871: Add abiltiy to save patches and load them as acceptable diffs #45

nocturnalastro · 2024-06-24T16:02:11Z

No description provided.

openshift-ci-robot · 2024-06-24T16:02:14Z

@nocturnalastro: This pull request references CNF-11871 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set.

In response to this:

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 openshift-eng/jira-lifecycle-plugin repository.

nocturnalastro · 2024-06-24T16:02:57Z

/cc @pixelsoccupied @AlinaSecret

openshift-ci · 2024-06-24T16:05:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from nocturnalastro. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

pixelsoccupied · 2024-06-24T16:10:17Z

pkg/compare/compare.go

@@ -173,6 +177,10 @@ func NewCmd(f kcmdutil.Factory, streams genericiooptions.IOStreams) *cobra.Comma
 			"In local mode will try to match all resources passed to the command")

 	cmd.Flags().StringVarP(&options.OutputFormat, "output", "o", "", fmt.Sprintf(`Output format. One of: (%s)`, strings.Join(OutputFormats, ", ")))
+
+	cmd.Flags().StringVarP(&options.PatchesSavePath, "patches-save-path", "p", "", "Path file where patches should be saved.")


Readme with an exmaple use case please!

So this cover what Dave was asking yeah? Giving customer to silence diffs overtime? (explanation of this would be nice under PR description/or README)

AlinaSecret

I think its enough for this review, still have some more comments about the tests but they may change according to the other comments so didn't add them for now

AlinaSecret · 2024-06-26T09:34:19Z

pkg/compare/compare.go

+			return err
+		}
+	} else {
+		o.patches = make(map[string][]string)


no need for the if and else clause:
o.patches = make(map[string][]string)
if o.PatchesLoadPath != "" {
o.patches, err = getPatches(o.PatchesLoadPath)
if err != nil {
return err
}
}

AlinaSecret · 2024-06-26T10:13:38Z

README.md

@@ -324,6 +324,18 @@ correlationSettings:

 ```

+#### Acceptable diffs
+
+It is possible save a set of diffs as patches. These can then be passed back into the tools as a set of acceptable diffs.


it is important that the user can set the known diffs, and not only be able to take all of the diffs and then use all of them as a patch, the user may would want only part of it.
the format of the patch is part of the api of the tool, the user need the ability to modify it by hand or at least some documentation about the format how it works. especially that the patches are wrapped by a mapping
i would add:

the external docs link on the patches

basic example on modify of the patch file
an example use ,

explanation about the format

AlinaSecret · 2024-06-26T10:40:41Z

pkg/compare/compare.go

@@ -114,6 +115,9 @@ type Options struct {
 	diffAll            bool
 	ShowManagedFields  bool
 	OutputFormat       string
+	PatchesSavePath    string


why do we need a save path and a load path?
i think instead you can have only a patch file and a re save flag (or better name)
because anyway if someone is using the load ad the save at the same time, the new patch will contain all of the patches specified in the loaded one.
what do you think? anyway i think that its not clear enough in the docs what happens if you use both of the save and load at the same time

AlinaSecret · 2024-06-26T10:43:10Z

pkg/compare/compare.go

+
+		patch, err := obj.getPatch()
+		if err != nil {
+			klog.Error(err)


as we discussed in a different pr context move to ignore errors

AlinaSecret · 2024-06-26T13:06:51Z

pkg/compare/compare.go

@@ -518,11 +583,41 @@ func (obj InfoObject) Name() string {
 	return slug.Make(apiKindNamespaceName(obj.clusterObj))
 }

+func (obj InfoObject) getPatch() (string, error) {
+	localRuntime, err := obj.Merged()


i am not sure if the patch should created from the version that already contains the patched version (like still not sure about the case if there is a load and save patch at the same time what will each of them include)

i would of expect here to use the object only with the ommited fields (without the load patch)

Yeah good point now that the other PR is merged I'll have to update this one :)

AlinaSecret · 2024-06-26T13:24:32Z

pkg/compare/compare.go

@@ -624,3 +719,20 @@ func (o Output) Print(format string, out io.Writer) (int, error) {
 	}
 	return n, nil
 }
+
+func (o Output) PrintPatches(out io.Writer) (int, error) {


i think its better to create a separate object for the patches, where has both the load function and the print functions. no need to use the ouptput that contains final output of the tool or the diff sum.

also because the patch file extends the api of the command, i think its better that is format is located in one place.
the code is a little to much divided between multiple places, that can make it hard to change and its harder to review.

so i suggest on creating a struct called PatchFile that has list/map of the patchsave object (that can be just patch)
the patch file object can contain the print function (i would call it export instead or save), and also a load function. even put all of the patch functions in a different file

AlinaSecret · 2024-06-26T13:27:06Z

pkg/compare/compare.go

@@ -455,26 +472,49 @@ func (o *Options) Run() error {
 			clusterObj:              &clusterCR,
 			FieldsToOmit:            o.ref.FieldsToOmit,
 		}
+		crName := apiKindNamespaceName(&clusterCR)
+		if patches, ok := o.patches[crName]; ok {


why are the patches mapped by a specific CR, instead of the corresponding reference.
it can be more stable to changes in the CRS name-namespace-apiversion-kind. and its logical that you would like to supresss a diff because of a field in the ref.

AlinaSecret · 2024-06-26T13:29:32Z

pkg/compare/compare_test.go

+	}()
+	cmd.Run(cmd, []string{})
+}
+
 // TestCompareRun ensures that Run command calls the right actions
 // and returns the expected error.
 func TestCompareRun(t *testing.T) {


please add a test that gets as input the patch file (that is saved in the tests directory) and tests that the diff is supressed, and same for the save option.
because the patch file is part of the api it should be fully tested as a separate file
also it will be super helpful for the review too see examples of the patch file

openshift-merge-robot · 2024-06-27T22:48:58Z

PR needs rebase.

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-sigs/prow repository.

nocturnalastro · 2024-06-28T10:25:48Z

/hold
while discuss the design

openshift-ci · 2024-07-18T14:27:14Z

@nocturnalastro: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/modtidy	`ebb2305`	link	true	`/test modtidy`
ci/prow/unit-report-creator	`ebb2305`	link	true	`/test unit-report-creator`
ci/prow/images	`ebb2305`	link	true	`/test images`

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-sigs/prow repository. I understand the commands that are listed here.

nocturnalastro · 2024-07-23T15:00:35Z

@AlinaSecret @pixelsoccupied Closing this one for now. I'll open up a new once once we've agreed on the design with the new implementation.

Add abiltiy to save patches and load them as acceptable diffs

a9bd7ce

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 24, 2024

openshift-ci bot requested review from AlinaSecret, pixelsoccupied and crwr45 June 24, 2024 16:03

pixelsoccupied reviewed Jun 24, 2024

View reviewed changes

Add patch commands example to README

ebb2305

AlinaSecret reviewed Jun 26, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 28, 2024

nocturnalastro closed this Jul 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CNF-11871: Add abiltiy to save patches and load them as acceptable diffs #45

CNF-11871: Add abiltiy to save patches and load them as acceptable diffs #45

nocturnalastro commented Jun 24, 2024

openshift-ci-robot commented Jun 24, 2024 •

edited by openshift-ci bot

Loading

nocturnalastro commented Jun 24, 2024

openshift-ci bot commented Jun 24, 2024

pixelsoccupied Jun 24, 2024 •

edited

Loading

AlinaSecret left a comment

AlinaSecret Jun 26, 2024

AlinaSecret Jun 26, 2024

AlinaSecret Jun 26, 2024

AlinaSecret Jun 26, 2024

AlinaSecret Jun 26, 2024

nocturnalastro Jun 27, 2024

AlinaSecret Jun 26, 2024

AlinaSecret Jun 26, 2024

AlinaSecret Jun 26, 2024

openshift-merge-robot commented Jun 27, 2024

nocturnalastro commented Jun 28, 2024

openshift-ci bot commented Jul 18, 2024

nocturnalastro commented Jul 23, 2024

CNF-11871: Add abiltiy to save patches and load them as acceptable diffs #45

CNF-11871: Add abiltiy to save patches and load them as acceptable diffs #45

Conversation

nocturnalastro commented Jun 24, 2024

openshift-ci-robot commented Jun 24, 2024 • edited by openshift-ci bot Loading

nocturnalastro commented Jun 24, 2024

openshift-ci bot commented Jun 24, 2024

pixelsoccupied Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

AlinaSecret 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-merge-robot commented Jun 27, 2024

nocturnalastro commented Jun 28, 2024

openshift-ci bot commented Jul 18, 2024

nocturnalastro commented Jul 23, 2024

openshift-ci-robot commented Jun 24, 2024 •

edited by openshift-ci bot

Loading

pixelsoccupied Jun 24, 2024 •

edited

Loading