fix: add spacebindingrequests to workspace binding list #387

mfrancisc · 2024-01-09T14:26:43Z

Users are unable to manage SpaceBindingRequests that are not provisioned ( meaning there is no SpaceBinding associated with it).

This PR adds the SpaceBindingRequests ( that have no SpaceBinding associated) to the list of Bindings in the Workspace, so that RHTAP users can manage them from the UI.

Jira: https://issues.redhat.com/browse/ASC-443

Related PR:

TODO:

Update E2E tests

filariow

great job! just a few nits

pkg/proxy/handlers/spacelister_get.go

pkg/proxy/proxy.go

pkg/proxy/handlers/spacelister_get.go

codecov · 2024-01-09T16:25:52Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (8e59443) 78.03% compared to head (dae3ca8) 78.40%.

Files	Patch %	Lines
pkg/proxy/handlers/spacelister_get.go	93.47%	4 Missing and 2 partials ⚠️
pkg/proxy/proxy.go	80.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #387      +/-   ##
==========================================
+ Coverage   78.03%   78.40%   +0.37%     
==========================================
  Files          38       38              
  Lines        3223     3306      +83     
==========================================
+ Hits         2515     2592      +77     
- Misses        616      620       +4     
- Partials       92       94       +2

Flag	Coverage Δ
unittests	`78.40% <92.15%> (+0.37%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Co-authored-by: Francesco Ilario <filario@redhat.com>

mfrancisc · 2024-01-10T09:08:47Z

/retest

filariow

lgtm

sadlerap

lgtm, but one optional thing that might be beneficial.

pkg/proxy/handlers/spacelister_get.go

alexeykazakov · 2024-01-10T19:37:51Z

pkg/proxy/handlers/spacelister_get.go

@@ -77,9 +79,16 @@ func GetUserWorkspace(ctx echo.Context, spaceLister *SpaceLister, workspaceName
 		ctx.Logger().Error(fmt.Sprintf("unauthorized access - there is no SpaceBinding present for the user %s and the workspace %s", userSignup.CompliantUsername, workspaceName))
 		return nil, nil
 	}
+
+	// list all SpaceBindingRequests , just in case there might be some failing to create a SpaceBinding resource.
+	allSpaceBindingRequests, err := listSpaceBindingRequestsForSpace(ctx, GetMembersFunc, space)


I'm a bit concerned about adding even more stuff to the GetUserWorkspace() function. This function is currently used not only in our WS lister API but it's called for every request to the proxy in case there is a workspace context in the request. And we don't care about failed SBRs in that case (or even about the available user roles either btw).
Maybe it's time to split this function to two? One to remain a lightweight version for the general proxy requests and and with all additional information for the WS lister. Or make this function flexible/configurable so it can be used everywhere.

Makes sense. Let me take a look an split it so that proxy request do not have to populate the bindings list. Thanks for the suggestion.

@alexeykazakov @MatousJobanek I've created 2 functions GetUserWorkspace used by proxy only and GetUserWorkspaceWithBindings used by the spacelister.

Please take a look when you have some time.

PS: there is one new error condition which is not tested 574d4eb#diff-23bf3528567b934bf6ab8d0d690dc61a0ac564ba1b43b89cb5ddde609bb150b1R75-R79 , I can add a specific test in the proxy_test.go as I'm not sure where that should go in the current TestProxy since it's more a combination of different headers and requests.

It's not that important where you add this test. Add it to whatever place you think makes sense. But let's add a test for it if it's not too tricky.

I've added a specific test for this error condition in 98d2640

Co-authored-by: Andy Sadler <ansadler@redhat.com>

# Conflicts: # go.mod # go.sum # pkg/proxy/proxy.go

# Conflicts: # go.mod # go.sum

alexeykazakov · 2024-01-17T22:29:58Z

pkg/proxy/handlers/spacelister_get.go

+	space, err := spaceLister.GetInformerServiceFunc().GetSpace(workspaceName)
+	if err != nil {
+		ctx.Logger().Error(errs.Wrap(err, "unable to get space"))
+		return userSignup, nil, nil


Why returning nil instead of err?

I've left the functionality unchanged

registration-service/pkg/proxy/handlers/spacelister_get.go

Lines 52 to 56 in 81de976

space, err := spaceLister.GetInformerServiceFunc().GetSpace(workspaceName)

if err != nil {

ctx.Logger().Error(errs.Wrap(err, "unable to get space"))

return nil, nil

}

I see we return 404 when when usersignup and space are not found.

Then let's add a comment to the function description stating that if no space found then a "nil" returned instead of an error.

makes sense. Done 52690f3

# Conflicts: # pkg/proxy/handlers/spacelister_test.go

alexeykazakov

Looks good overall but it looks like we could add more unit tests.
Could you please take a look at the codecov report and cover the missing cases when it's not too tricky?

filariow

lgtm

mfrancisc · 2024-02-02T10:52:51Z

Could you please take a look at the codecov report and cover the missing cases when it's not too tricky?

@alexeykazakov thanks for pointing that out, I've added some more unit tests to cover the error cases, please take a look at 1c1fa6f

alexeykazakov

Looks good. Thanks.

openshift-ci · 2024-02-02T16:33:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeykazakov, filariow, mfrancisc, sadlerap

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

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

sonarqubecloud · 2024-02-05T07:12:21Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

8 New issues
0 Security Hotspots
No data about Coverage
9.3% Duplication on New Code

See analysis details on SonarCloud

add spacebindingrequests to workspace binding list

6c71351

mfrancisc requested review from sadlerap and filariow January 9, 2024 14:26

mfrancisc requested review from MatousJobanek, xcoulon, alexeykazakov, rajivnathan, ranakan19 and sbryzak as code owners January 9, 2024 14:26

openshift-ci bot requested a review from michaelkleinhenz January 9, 2024 14:26

Merge branch 'master' into fixbindinglist

d6c6d77

filariow reviewed Jan 9, 2024

View reviewed changes

pkg/proxy/handlers/spacelister_get.go Outdated Show resolved Hide resolved

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

pkg/proxy/proxy.go Show resolved Hide resolved

pkg/proxy/handlers/spacelister_get.go Outdated Show resolved Hide resolved

mfrancisc and others added 2 commits January 10, 2024 09:51

Update pkg/proxy/handlers/spacelister_get.go

ea8a905

Co-authored-by: Francesco Ilario <filario@redhat.com>

Update pkg/proxy/handlers/spacelister_get.go

a4346c3

Co-authored-by: Francesco Ilario <filario@redhat.com>

mfrancisc added 3 commits January 10, 2024 10:12

fix context

7619769

fix linter

dfd747f

pass getMembersFunc from proxy

9095d1c

filariow approved these changes Jan 10, 2024

View reviewed changes

mfrancisc mentioned this pull request Jan 10, 2024

fix: add spacebindingrequests to workspace binding list codeready-toolchain/toolchain-e2e#877

Merged

sadlerap approved these changes Jan 10, 2024

View reviewed changes

pkg/proxy/handlers/spacelister_get.go Outdated Show resolved Hide resolved

alexeykazakov reviewed Jan 10, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase label Jan 11, 2024

mfrancisc and others added 4 commits January 15, 2024 18:11

Update pkg/proxy/handlers/spacelister_get.go

f3a26ad

Co-authored-by: Andy Sadler <ansadler@redhat.com>

split GetUserWorkspace function

574d4eb

Merge branch 'master' into fixbindinglist

f9f1466

# Conflicts: # go.mod # go.sum # pkg/proxy/proxy.go

Merge branch 'master' into fixbindinglist

3613b52

# Conflicts: # go.mod # go.sum

openshift-merge-robot removed the needs-rebase label Jan 17, 2024

alexeykazakov reviewed Jan 17, 2024

View reviewed changes

mfrancisc and others added 4 commits January 18, 2024 11:53

add test for multiple spacebindings

98d2640

some fixes, add tests for new GetUserWorkspace function

52e236b

fix comment

52690f3

Merge branch 'master' into fixbindinglist

5a12c9c

openshift-merge-robot added the needs-rebase label Jan 31, 2024

Merge branch 'master' into fixbindinglist

d9cf43f

# Conflicts: # pkg/proxy/handlers/spacelister_test.go

openshift-merge-robot removed the needs-rebase label Feb 1, 2024

alexeykazakov reviewed Feb 2, 2024

View reviewed changes

filariow approved these changes Feb 2, 2024

View reviewed changes

address missing error cases in unit tests

1c1fa6f

alexeykazakov approved these changes Feb 2, 2024

View reviewed changes

openshift-ci bot added the approved label Feb 2, 2024

Merge branch 'master' into fixbindinglist

dae3ca8

mfrancisc merged commit 545695d into codeready-toolchain:master Feb 5, 2024
11 of 12 checks passed

mfrancisc deleted the fixbindinglist branch February 5, 2024 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add spacebindingrequests to workspace binding list #387

fix: add spacebindingrequests to workspace binding list #387

mfrancisc commented Jan 9, 2024 •

edited by alexeykazakov

Loading

filariow left a comment

codecov bot commented Jan 9, 2024 •

edited

Loading

mfrancisc commented Jan 10, 2024

filariow left a comment

sadlerap left a comment

alexeykazakov Jan 10, 2024

mfrancisc Jan 15, 2024

mfrancisc Jan 17, 2024

alexeykazakov Jan 17, 2024

mfrancisc Jan 18, 2024

alexeykazakov Jan 17, 2024

mfrancisc Jan 18, 2024

alexeykazakov Jan 18, 2024

mfrancisc Jan 18, 2024

alexeykazakov left a comment

filariow left a comment

mfrancisc commented Feb 2, 2024

alexeykazakov left a comment

openshift-ci bot commented Feb 2, 2024

sonarqubecloud bot commented Feb 5, 2024

	space, err := spaceLister.GetInformerServiceFunc().GetSpace(workspaceName)
	if err != nil {
	ctx.Logger().Error(errs.Wrap(err, "unable to get space"))
	return nil, nil
	}

fix: add spacebindingrequests to workspace binding list #387

fix: add spacebindingrequests to workspace binding list #387

Conversation

mfrancisc commented Jan 9, 2024 • edited by alexeykazakov Loading

filariow left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 9, 2024 • edited Loading

Codecov Report

mfrancisc commented Jan 10, 2024

filariow left a comment

Choose a reason for hiding this comment

sadlerap 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

alexeykazakov left a comment

Choose a reason for hiding this comment

filariow left a comment

Choose a reason for hiding this comment

mfrancisc commented Feb 2, 2024

alexeykazakov left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 2, 2024

sonarqubecloud bot commented Feb 5, 2024

Quality Gate passed

mfrancisc commented Jan 9, 2024 •

edited by alexeykazakov

Loading

codecov bot commented Jan 9, 2024 •

edited

Loading