Refactor decouple #188

acmenezes · 2022-08-30T14:26:19Z

This refactor is supposed to revert the potential risk for redundant circular reference caused by importing from operator package options or flags that belong to the command level. At the same time avoid the risk of having a tight coupling trying to pass all options to the operator package that doesn't care about most of them.

Now new functionality like adding CRs from the command line are very hard to implement. It's definitely a no go having to change the package on the bottom of the stack to implement a new flag that will be used on middle of the stack.

The flow of opcap packages is designed to be check --> consumes --> capability --> that consumes --> operator. It is still not perfect. But we should avoid as much as possible trying to pass parameters across the packages to the bottom of the stack or importing in reverse order which may lead to circular reference when we need to add new functionality in the future.

I've also took the opportunity to eliminate some unnecessary code a make the interfaces mode clear by not exporting a bunch of things that were unnecessarily exported on the capability package.

Here is what's being removed from the operator package:

type OperatorCheckOptions struct {
	// AuditPlan is an ordered list of tests to be run
	// during an operator audit
	AuditPlan []string
	// CatalogSource provides target catalog source
	// from which to list package manifests
	CatalogSource string
	// CatalogSourceNamespace specifies the namespace of the
	// catalog source to be used
	CatalogSourceNamespace string
	// ListPackages operation lists packages in the catalog source
	ListPackages bool
	// FilterPackages provides a list of packages to find
	FilterPackages []string
	// AllInstallModes is passed to test all install modes
	AllInstallModes bool
}

The flags should be processed on the top of the stack and not on the bottom.

We still need to redo the all install mode strategy though.

This change eliminates redundancy and tight coupling. This reverts commit 96948d8, reversing changes made to 67291a4.

On this commit all capAudit objects and methods were changed to lower case in order to be non exported. The capAudit interface was removed also to prevent confusion once the only interface to this package is CapAuditor and not capAudit. CapAuditor now should be upper case and exported. More on the documentation commit. Signed-off-by: acmenezes <[email protected]>

We don't need a builder for this simple object and buildWorkQueue can be run as the first step of an audit. That makes the code simpler and easier to understand. All flag data interesting to capability package can be passed by using the CapAuditor object and add to it as needed. It makes CapAuditor the single entry point of flag data to the capabiltiy package. Signed-off-by: acmenezes <[email protected]>

internal/capability/auditor.go

skattoju

looks good other than some cosmetic nits

OchiengEd · 2022-08-30T16:15:04Z

cmd/check.go

@@ -90,5 +100,4 @@ func init() {
 	flags.StringSliceVar(&checkflags.AuditPlan, "auditplan", defaultAuditPlan, "audit plan is the ordered list of operator test functions to be called during a capability audit.")
 	flags.BoolVar(&checkflags.ListPackages, "list-packages", false, "list packages in the catalog")
 	flags.StringSliceVar(&checkflags.FilterPackages, "filter-packages", []string{}, "a list of package(s) which limits audits and/or other flag(s) output")
-	flags.BoolVar(&checkflags.AllInstallModes, "all-installmodes", false, "when set, all install modes supported by an operator will be tested")


Take note of any functionality removed. Here I can see the functionality to test all install modes has been removed.

Create a new issue if you want this reworked at a future time.

Totally agree. Here is the new issue: Enable all supported install modes testing #189

@acmenezes , regarding the name convention for parameters, should we create them with the - (i.e --list-packages) or not like --auditplan?

My vote would be to include the '-'.

There we go 9929e85 - dash included.

bcrochet

Just a few comments.

cmd/check.go

internal/capability/auditor.go

internal/capability/operand_cleanup.go

internal/capability/operator_cleanup.go

internal/capability/operator_install.go

internal/capability/report.go

yashoza19

Everything else looks great. Tested.

cmd/check.go

Signed-off-by: acmenezes <[email protected]>

This allow for testing as well as other implementations of writers such as writing to databases when we develop a more solid reporting mechanism. - Reports now are simpler functions taking io.writer - operand and operator install will handle file creation and where to write reports to Signed-off-by: acmenezes <[email protected]>

bcrochet

A few changes and general comments.

internal/capability/auditor.go

bcrochet · 2022-09-01T12:47:39Z

cmd/check.go

@@ -90,5 +100,4 @@ func init() {
 	flags.StringSliceVar(&checkflags.AuditPlan, "auditplan", defaultAuditPlan, "audit plan is the ordered list of operator test functions to be called during a capability audit.")
 	flags.BoolVar(&checkflags.ListPackages, "list-packages", false, "list packages in the catalog")
 	flags.StringSliceVar(&checkflags.FilterPackages, "filter-packages", []string{}, "a list of package(s) which limits audits and/or other flag(s) output")
-	flags.BoolVar(&checkflags.AllInstallModes, "all-installmodes", false, "when set, all install modes supported by an operator will be tested")


My vote would be to include the '-'.

internal/capability/auditor.go

internal/capability/operand_cleanup.go

internal/capability/operand_install.go

bcrochet · 2022-09-01T13:09:46Z

internal/capability/operand_install.go

@@ -105,7 +106,15 @@ func (ca *CapAudit) OperandInstall() error {
 		logger.Debug("exiting OperandInstall since no ALM_Examples found in CSV")
 	}

-	ca.Report(OperandInstallRptOptionFile{FilePath: "operand_install_report.json"}, OperandInstallRptOptionPrint{})
+	file, err := os.OpenFile("operand_install_report.json", os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)


Creating a report doesn't seem to be the job of this function. It should either be passed in an io.Reader, or return the data for someone else to log/report.

This is not a blocker. Just wanted to point this out for future discussion.

Hi @bcrochet the ca or capAudit object contains the data. And the method. And it's lifecycle ends with this install function. What do you suggest?

Don't have a suggestion just yet. I need to get a fuller understanding of the flow here.

bcrochet · 2022-09-01T13:11:24Z

internal/capability/operator_install.go

 		logger.Debugf("Error creating subscriptions: %w", err)
 		return err
 	}

 	// Get a Succeeded or Failed CSV with one minute timeout
-	ca.Csv, err = ca.Client.GetCompletedCsvWithTimeout(ca.Namespace, ca.CsvWaitTime)
+	var err error
+	ca.csv, err = ca.client.GetCompletedCsvWithTimeout(ca.namespace, ca.csvWaitTime)


These two lines can be collaped, and just have 'ca.csv, err := ...'

Hi @bcrochet apparently VSCode doesn't like that way. That's what happens when we remove and try to collapse the lines. **

**
Didn't have time to troubleshoot this. Do you have any suggestions?

Leave it for now. Not a blocker.

internal/capability/operator_install.go

internal/capability/report.go

…ntation Signed-off-by: acmenezes <[email protected]>

Signed-off-by: acmenezes <[email protected]>

internal/operator/subscription.go

Signed-off-by: acmenezes <[email protected]>

bcrochet

/lgtm

exe-prow-github-app · 2022-09-01T17:18:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acmenezes, bcrochet, skattoju, yashoza19

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~~ [acmenezes,yashoza19]

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

exe-prow-github-app · 2022-09-01T17:18:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acmenezes, bcrochet, skattoju, yashoza19

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~~ [acmenezes,yashoza19]

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

acmenezes added 3 commits August 29, 2022 15:19

Revert Operator Check Options

0261179

This change eliminates redundancy and tight coupling. This reverts commit 96948d8, reversing changes made to 67291a4.

acmenezes requested review from OchiengEd, mrhillsman, yashoza19 and skattoju August 30, 2022 14:26

exe-prow-github-app bot requested review from rocrisp, itroyano and spabba17 August 30, 2022 14:26

exe-prow-github-app bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 30, 2022

acmenezes added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 30, 2022

mrhillsman removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2022

skattoju reviewed Aug 30, 2022

View reviewed changes

internal/capability/auditor.go Outdated Show resolved Hide resolved

skattoju reviewed Aug 30, 2022

View reviewed changes

internal/capability/auditor.go Show resolved Hide resolved

skattoju reviewed Aug 30, 2022

View reviewed changes

internal/capability/auditor.go Outdated Show resolved Hide resolved

skattoju approved these changes Aug 30, 2022

View reviewed changes

exe-prow-github-app bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 30, 2022

OchiengEd reviewed Aug 30, 2022

View reviewed changes

bcrochet requested changes Aug 30, 2022

View reviewed changes

yashoza19 approved these changes Aug 30, 2022

View reviewed changes

cmd/check.go Outdated Show resolved Hide resolved

Replace s for subscriptions, reads better

d2dd35c

Signed-off-by: acmenezes <[email protected]>

exe-prow-github-app bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 30, 2022

acmenezes added 2 commits August 30, 2022 15:05

Rename filter-packages to packages, simpler and intuitive

5adfeaf

Signed-off-by: acmenezes <[email protected]>

REMOVE unnecessary blak lines

2f3487a

Signed-off-by: acmenezes <[email protected]>

acmenezes added 4 commits August 30, 2022 15:35

CLEANUP code by breaking and combining lines

0f94b3e

Signed-off-by: acmenezes <[email protected]>

CATCH ignored error from CreateNamespace

39d5f16

Signed-off-by: acmenezes <[email protected]>

COMBINE lines for better reading

d9bc1ba

Signed-off-by: acmenezes <[email protected]>

bcrochet requested changes Sep 1, 2022

View reviewed changes

bcrochet mentioned this pull request Sep 1, 2022

Added log package #177

Merged

4 tasks

acmenezes added 6 commits September 1, 2022 11:00

CHANGE auditplan to audit-plan for better reading

9929e85

REMOVE auditor interface as it doesn't currently have another impleme…

18d4706

…ntation Signed-off-by: acmenezes <[email protected]>

FIX comment capitalization on WorkQueue word

00f1e49

REMOVE log fatal from internal package

e0e7aa3

Signed-off-by: acmenezes <[email protected]>

REPLACE printf by logger.Debugf

36e1899

Signed-off-by: acmenezes <[email protected]>

REMOVE unnecessary else clause

63dceab

Signed-off-by: acmenezes <[email protected]>

bcrochet reviewed Sep 1, 2022

View reviewed changes

internal/operator/subscription.go Outdated Show resolved Hide resolved

acmenezes added 2 commits September 1, 2022 11:51

REMOVE unnecessary code indentation and capitalized error msg

349c93a

Signed-off-by: acmenezes <[email protected]>

REMOVE unecessary elses and return early

8d227c2

Signed-off-by: acmenezes <[email protected]>

exe-prow-github-app bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 1, 2022

acmenezes force-pushed the refactor_decouple branch 2 times, most recently from 9218f0b to 8d227c2 Compare September 1, 2022 16:20

bcrochet approved these changes Sep 1, 2022

View reviewed changes

exe-prow-github-app bot assigned bcrochet Sep 1, 2022

exe-prow-github-app bot added the lgtm Indicates that a PR is ready to be merged. label Sep 1, 2022

yashoza19 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2022

exe-prow-github-app bot merged commit 31757cd into opdev:main Sep 1, 2022

This was referenced Sep 2, 2022

Refactor - Decouple internal packages from command level code #182

Closed

Review methods that should and shouldn't be exported #99

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor decouple #188

Refactor decouple #188

acmenezes commented Aug 30, 2022

skattoju left a comment

OchiengEd Aug 30, 2022

acmenezes Aug 30, 2022

yuriolisa Aug 31, 2022

bcrochet Sep 1, 2022

acmenezes Sep 1, 2022

bcrochet left a comment

yashoza19 left a comment

bcrochet left a comment

bcrochet Sep 1, 2022

bcrochet Sep 1, 2022

bcrochet Sep 1, 2022

acmenezes Sep 1, 2022

bcrochet Sep 1, 2022

bcrochet Sep 1, 2022

acmenezes Sep 1, 2022

bcrochet Sep 1, 2022

bcrochet left a comment

exe-prow-github-app bot commented Sep 1, 2022

exe-prow-github-app bot commented Sep 1, 2022

Refactor decouple #188

Refactor decouple #188

Conversation

acmenezes commented Aug 30, 2022

skattoju 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

bcrochet left a comment

Choose a reason for hiding this comment

yashoza19 left a comment

Choose a reason for hiding this comment

bcrochet 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

bcrochet left a comment

Choose a reason for hiding this comment

exe-prow-github-app bot commented Sep 1, 2022

exe-prow-github-app bot commented Sep 1, 2022