馃尡 test/e2e/virtual/apiexport: make authorizer test self-contained #2611
Conversation
s-urbaniak
force-pushed
the
self-contained-e2e
branch
from
db638e3
to
5a31bae
Compare
| export, err := user1KcpClient.Cluster(serviceProvider1Path).ApisV1alpha1().APIExports().Get(ctx, "wild.wild.west", metav1.GetOptions{}) | ||
| require.NoError(t, err, "error getting APIExport %s|%s", serviceProvider1Path, export.Name) | ||
| t.Logf("install sherriffs API resource schema, API export, permissions for user-3 to be able to bind to the export in service provider workspace %q", serviceProvider1Path) | ||
| require.NoError(t, apply(t, ctx, serviceProvider1Path, framework.UserConfig("user-1", rest.CopyConfig(cfg)), |
There was a problem hiding this comment.
Why are we doing this as user-1?
There was a problem hiding this comment.
user-1 is admin of serviceprovider workspace
There was a problem hiding this comment.
Yikes, hope we can figure out how to dynamically inject more users in the future at runtime so that we can make these semantically useful. Can you track this as a follow-up for e2e in general?
| require.NoError(t, err, "error getting APIExport %s|%s", serviceProvider1Path, export.Name) | ||
| t.Logf("install sherriffs API resource schema, API export, permissions for user-3 to be able to bind to the export in service provider workspace %q", serviceProvider1Path) | ||
| require.NoError(t, apply(t, ctx, serviceProvider1Path, framework.UserConfig("user-1", rest.CopyConfig(cfg)), | ||
| &apisv1alpha1.APIResourceSchema{ |
There was a problem hiding this comment.
Don't we auto-gen an APIRS from the Go types? Are we using something special here for a reason?
There was a problem hiding this comment.
there are no go types for these test types (sherriffs/cowboys) available. these types are just for testing, we don't maintain them.
There was a problem hiding this comment.
test/e2e/fixtures/wildwest/apis/wildwest/v1alpha1/types.go:type Cowboy struct {
?
There was a problem hiding this comment.
he, i had no idea, however do we want to depend on central fixtures? i was having the impression we want to get rid of it.
There was a problem hiding this comment.
using those types and clients is fine IMO. "getting rid of the fixtures" is about helper functions built around those types. We don't want them.
There was a problem hiding this comment.
And rule of thumb: don't build anything special into shared fixtures. If you need them to change, chance is high that you better make a copy.
| Spec: apisv1alpha1.APIBindingSpec{ | ||
| Reference: apisv1alpha1.BindingReference{ | ||
| Export: &apisv1alpha1.ExportBindingReference{ | ||
| Path: serviceProvider1Path.String(), |
There was a problem hiding this comment.
nit: if serviceProvider1 is for sherrifs and serviceProvider2 is cowboys, can we update to sherrifsProvider and cowboysProvider?
| return groupExists(groups, wildwest.GroupName) && groupExists(groups, "wild.wild.west"), nil | ||
| }) | ||
| require.NoError(t, err) | ||
| return groupExists(groups, wildwest.GroupName) && groupExists(groups, "wild.wild.west"), "" |
There was a problem hiding this comment.
return the missing but expected groups, so this errors nicely
There was a problem hiding this comment.
let's do this as a follow-up, i didn't change the groupExists method here.
There was a problem hiding this comment.
Sounds good - track the follow-ups however you want.
| dynamicResource = dynamicClient.Cluster(workspace).Resource(mapping.Resource) | ||
| } | ||
|
|
||
| bytes, err := json.Marshal(obj) |
There was a problem hiding this comment.
nit: use the scheme
There was a problem hiding this comment.
wdym, do you have a reference?
There was a problem hiding this comment.
You want a structured encoder from a scheme that has the type registered - I don't have a good link but would just auto-complete my way there.
s-urbaniak
force-pushed
the
self-contained-e2e
branch
from
5a31bae
to
472bc77
Compare
|
All remaining comments are minor and can be tackled in follow-ups. /lgtm |
s-urbaniak
added
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: stevekuznetsov 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:
Approvers can indicate their approval by writing |
|
/refresh |
Summary
This makes the e2e test for virtual apiexport authorizer self-contained. The
applymethod will be used to add new e2e tests asserting existing issues.Related issue(s)