Bug 1961317: Adding static assets to related objects #175

Elbehery · 2021-06-07T20:34:51Z

This PR parses operator static assets, into operator related objects.

This PR depends on openshift/library-go#1099

openshift-ci · 2021-06-07T20:34:57Z

@Elbehery: This pull request references Bugzilla bug 1961317, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.8.0) matches configured target release for branch (4.8.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (wduan@redhat.com), skipping review request.

In response to this:

Bug 1961317: Adding static assets to related objects

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.

jsafrane · 2021-06-08T07:28:14Z

pkg/operator/csidriveroperator/driverstarter.go

-	running bool
+	mgr                manager.ControllerManager
+	running            bool
+	ctrlRelatedObjects func() []configv1.ObjectReference


add error return to the function

jsafrane · 2021-06-08T07:28:19Z

pkg/operator/csidriveroperator/driverstarter.go

+	objs, err := src.RelatedObjects()
+	if err != nil {
+		// TODO : error msg or aggregate error
+		klog.Errorf("Can not find RelatedObjects, got %v", err)
+	} else {
+		ctrlRelatedObjects = func() []configv1.ObjectReference {
+			return objs
+		}
+	}


Please move src.RelatedObjects call to returned func() ([]configv1.ObjectReference, error) and call it during sync() - it can have proper error handling there and automatic retry on random API server error.

RelatedObject( ) can be only called from StaticResourceController .. Since we use type ControllerManager within csiDriverControllerManager , its not possible to call it within Sync()

actually thats why we added the funktor, so it has the related objects within the scope of Sync() and then we get the data from it, afterwards the funktor it self is garbage collected

ctrlRelatedObjects = func() ([]configv1.ObjectReference, error) { return src.RelatedObjects() }

Ahh I c, I thought u want everything in Sync .. ok On it 👍🏽

@jsafrane Done 👍🏽

deads2k · 2021-06-08T12:41:28Z

pkg/operator/csidriveroperator/driverstarter.go

-	running bool
+	mgr                manager.ControllerManager
+	running            bool
+	ctrlRelatedObjects func() ([]configv1.ObjectReference, error)


This looks worthy of an interface to me. I won't stick on the first one I see, but I'm likely to stick on it when we grow another.

+1 👍🏽

Please make an interface for this, the rest is lgtm

Elbehery · 2021-06-11T20:18:25Z

/retest

Elbehery · 2021-06-12T06:28:16Z

/retest

openshift-bot · 2021-06-14T08:26:55Z

/bugzilla refresh

The main branch will open for development of next OCP version. Recalculating validity of PRs linked to this PR.

openshift-ci · 2021-06-14T08:28:02Z

@openshift-bot: This pull request references Bugzilla bug 1961317, which is invalid:

expected the bug to target the "4.9.0" release, but it targets "4.8.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

The main branch will open for development of next OCP version. Recalculating validity of PRs linked to this PR.

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-bot · 2021-06-14T08:28:35Z

/bugzilla refresh

The main branch will open for development of next OCP version. Recalculating validity of PRs linked to this PR.

openshift-ci · 2021-06-14T08:29:39Z

@openshift-bot: This pull request references Bugzilla bug 1961317, which is invalid:

expected the bug to target the "4.9.0" release, but it targets "4.8.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

The main branch will open for development of next OCP version. Recalculating validity of PRs linked to this PR.

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.

Elbehery · 2021-06-14T08:33:39Z

/bugzilla refresh

openshift-ci · 2021-06-14T08:33:42Z

@Elbehery: This pull request references Bugzilla bug 1961317, which is invalid:

expected the bug to target the "4.9.0" release, but it targets "4.8.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

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.

jsafrane · 2021-06-14T14:51:08Z

pkg/operator/csidriveroperator/driverstarter.go

@@ -152,7 +160,7 @@ func (c *CSIDriverStarterController) sync(ctx context.Context, syncCtx factory.S
 				Name:     ctrl.operatorConfig.CSIDriverName,
 			})
 			// add static assets
-			objs, err := ctrl.ctrlRelatedObjects()
+			objs, err := ctrl.getRelatedObjects()


Not sure if it's worth having separate getRelatedObjects function, you can call ctrl.ctrlRelatedObjects,RelatedObjects() directly here.

jsafrane · 2021-06-15T07:44:20Z

/retest

jsafrane · 2021-06-15T10:38:56Z

/lgtm

openshift-ci · 2021-06-15T10:39:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Elbehery, jsafrane

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

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

openshift-bot · 2021-06-16T01:00:51Z

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

openshift-ci · 2021-06-16T01:00:53Z

@openshift-bot: This pull request references Bugzilla bug 1961317, which is invalid:

expected the bug to target the "4.9.0" release, but it targets "4.8.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

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-bot · 2021-06-16T01:28:38Z

/retest

Please review the full test history for this PR and help us cut down flakes.

Elbehery · 2021-06-16T16:01:16Z

/bugzilla refresh

openshift-ci · 2021-06-16T16:01:22Z

@Elbehery: This pull request references Bugzilla bug 1961317, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (wduan@redhat.com), skipping review request.

In response to this:

/bugzilla refresh

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-bot · 2021-06-16T18:28:31Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-16T22:16:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-17T05:36:17Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-06-17T08:12:30Z

@Elbehery: All pull requests linked via external trackers have merged:

Bugzilla bug 1961317 has been moved to the MODIFIED state.

In response to this:

Bug 1961317: Adding static assets to related objects

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 bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. label Jun 7, 2021

openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jun 7, 2021

openshift-ci bot requested review from bertinatto and huffmanca June 7, 2021 20:35

Elbehery mentioned this pull request Jun 7, 2021

Bug 1961317: Adding static assets to related objects openshift/library-go#1099

Merged

Elbehery force-pushed the add_related_objects branch from 642d904 to 20cb777 Compare June 7, 2021 20:47

jsafrane reviewed Jun 8, 2021

View reviewed changes

Elbehery force-pushed the add_related_objects branch from 20cb777 to 2e96b3d Compare June 8, 2021 12:31

deads2k reviewed Jun 8, 2021

View reviewed changes

Elbehery force-pushed the add_related_objects branch from 2e96b3d to 0c32190 Compare June 10, 2021 08:00

Elbehery mentioned this pull request Jun 11, 2021

Add rbac related objects #174

Closed

openshift-ci bot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Jun 14, 2021

Elbehery added 2 commits June 14, 2021 16:47

Bug 1961317: Adding static assets to related objects

e42e565

Updating vendor dependencies

3db227b

jsafrane reviewed Jun 14, 2021

View reviewed changes

refactor to interface

464599c

Elbehery force-pushed the add_related_objects branch from c60a431 to 464599c Compare June 14, 2021 14:56

openshift-ci bot assigned jsafrane Jun 15, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 15, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 2021

openshift-ci bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jun 16, 2021

openshift-merge-robot merged commit 3533155 into openshift:master Jun 17, 2021

openshift-ci bot mentioned this pull request Jan 13, 2022

Bug 1961317: Add operator's ClusterRoleBinding to RelatedObjects #251

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1961317: Adding static assets to related objects #175

Bug 1961317: Adding static assets to related objects #175

Elbehery commented Jun 7, 2021

openshift-ci bot commented Jun 7, 2021

jsafrane Jun 8, 2021

jsafrane Jun 8, 2021

Elbehery Jun 8, 2021

Elbehery Jun 8, 2021

jsafrane Jun 8, 2021

Elbehery Jun 8, 2021

Elbehery Jun 8, 2021

deads2k Jun 8, 2021

Elbehery Jun 8, 2021

jsafrane Jun 14, 2021

Elbehery commented Jun 11, 2021

Elbehery commented Jun 12, 2021

openshift-bot commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021

openshift-bot commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021

Elbehery commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021

jsafrane Jun 14, 2021

Elbehery Jun 14, 2021

jsafrane commented Jun 15, 2021

jsafrane commented Jun 15, 2021

openshift-ci bot commented Jun 15, 2021

openshift-bot commented Jun 16, 2021

openshift-ci bot commented Jun 16, 2021

openshift-bot commented Jun 16, 2021

Elbehery commented Jun 16, 2021

openshift-ci bot commented Jun 16, 2021

openshift-bot commented Jun 16, 2021

openshift-bot commented Jun 16, 2021

openshift-bot commented Jun 17, 2021

openshift-ci bot commented Jun 17, 2021

Bug 1961317: Adding static assets to related objects #175

Bug 1961317: Adding static assets to related objects #175

Conversation

Elbehery commented Jun 7, 2021

openshift-ci bot commented Jun 7, 2021

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

Elbehery commented Jun 11, 2021

Elbehery commented Jun 12, 2021

openshift-bot commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021

openshift-bot commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021

Elbehery commented Jun 14, 2021

openshift-ci bot commented Jun 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane commented Jun 15, 2021

jsafrane commented Jun 15, 2021

openshift-ci bot commented Jun 15, 2021

openshift-bot commented Jun 16, 2021

openshift-ci bot commented Jun 16, 2021

openshift-bot commented Jun 16, 2021

Elbehery commented Jun 16, 2021

openshift-ci bot commented Jun 16, 2021

openshift-bot commented Jun 16, 2021

openshift-bot commented Jun 16, 2021

openshift-bot commented Jun 17, 2021

openshift-ci bot commented Jun 17, 2021