Add APIBinding controller, replace inheritFrom with APIBindings #755
Conversation
d242ca5 to
2fd45fc
Compare
| return ws.Status.Phase == tenancyv1alpha1.ClusterWorkspacePhaseReady | ||
| }, wait.ForeverTestTimeout, 100*time.Millisecond, "error waiting for source workspace to be ready") | ||
|
|
||
| server.Artifact(t, func() (runtime.Object, error) { |
There was a problem hiding this comment.
(No action required) I don't actually know what these artifact stanzas buy us. Does anyone actually use them?
| _, err = kcpClients.Cluster(targetWorkspaceClusterName).ApisV1alpha1().APIBindings().Create(ctx, apiBinding, metav1.CreateOptions{}) | ||
| require.NoError(t, err) | ||
|
|
||
| t.Logf("Make sure %s API group does NOT show up in workspace %q group discovery", wildwest.GroupName, sourceWorkspace.Name) |
There was a problem hiding this comment.
(No action required) Aha, so exporting a type doesn't require it to be installed in a workspace? And an apiresourceschema is basically a crd that isn't reconciled into an api without an apibinding?
There was a problem hiding this comment.
A APIResourceSchema is like a snapshot (in time) of a CRD, but yes: it is not served in that workspace. The objects are in the consumer workspaces.
29a75b5 to
a506143
Compare
Schema evolution is not yet part of the implementation. |
There was a problem hiding this comment.
Phew, apibinding_reconcile reviewed. Only its unit testing to go.
It appears there may be conflicts between wanting to retry while also wanting to set status? These conflicts don't surface in the current unit tests because they involve the controller and there are no tests of the controller and the error paths. Maybe refactor to ensure future conflicts of this nature can be caught by tests rather than in review?
I'm talking about the file |
|
Something about the precedence would make sense. On the other hand, I would prefer to build something that prevent conflicts in the first place, see #748. |
There was a problem hiding this comment.
Phew, apibinding_reconcile reviewed. Only its unit testing to go.
It appears there may be conflicts between wanting to retry while also wanting to set status? The methods in which the problems appear are complicated enough that discovering these kinds of errors in test would be challenging if not possible. Not suggesting that the controller requires a refactor in this PR, but I would hope for future iteration towards a more testable implementation.
| return | ||
| } | ||
|
|
||
| require.Equal(t, status, tc.wantReconcileStatus) |
There was a problem hiding this comment.
(No action required) Maybe use RequireNoDiff (moving to test/common first) to ensure better error messages when status differs?
| } | ||
|
|
||
| func TestWorkspaceAPIExportReferenceReconciler(t *testing.T) { | ||
| tests := map[string]struct { |
There was a problem hiding this comment.
(No action required) The complexity of test case definition suggests that the functionality under test would benefit from refactoring for testability. If not in this PR, then in the near future.
| return b | ||
| } | ||
|
|
||
| func (b *bindingBuilder) WithWorkspaceReference(workspaceName, exportName string) *bindingBuilder { |
There was a problem hiding this comment.
(No action required) I think part of the advantage of builders is removing the need to parametrize them where possible. Ideally the cases would be as simple as WithSomeWorkspaceReference and WithAnotherWorkspaceReference with correspondence across builders (i.e. SomeWorkspace and AnotherWorkspace constants). This minimizes the details in the test cases to the minimum (Some, Another, Different, etc) required to clearly indicate intent.
Added to #417 as follow-up. The whole rebinding is not done yet. |
It is tested, it is the core of the APIBindings being available as types in workspaces. Same for CRD, both custom and system CRD. This is used everywhere. |
cf71cd0 to
aa05066
Compare
Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
…c.type lookup for CRDs
Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>
0d48633 to
ad73cbf
Compare
Continues and replaces #702. All comments addressed, or follow-ups added to #417.
Also fixes #731.
Majority of the work by @ncdc, copied from #702.