rollouts: support repoSync option
#3850
Conversation
natasha41575
force-pushed
the
rollouts/reposync
branch
2 times, most recently
from
1d324c0
to
41c62f2
Compare
natasha41575
force-pushed
the
rollouts/reposync
branch
4 times, most recently
from
03cf85e
to
27a28e2
Compare
|
@droot I am separately working on making this code unit-testable and still want to do some more manual testing on this, but I think this PR is in decent shape for review. |
rollouts/config/rbac/role.yaml
Outdated
| verbs: | ||
| - update | ||
| - apiGroups: | ||
| - gitops.kpt.dev | ||
| resources: | ||
| - remoterootsyncs/status | ||
| - remotesyncs/status |
There was a problem hiding this comment.
In our getting-started-guide, we setup the RBAC permissions on target clusters, we need to update it to include RepoSync resources as well.
Keep a note or issue somewhere for updating it once this is merged.
There was a problem hiding this comment.
Looks like that getting started doc already has reposyncs under the rbac permissions.
| rrs.Spec.Template.Metadata) { | ||
| rrs.Spec.Template.Spec.Git.Revision = pkg.Revision | ||
| rrs.Spec.Template.Metadata = rollout.Spec.SyncTemplate.RootSync.Metadata | ||
| metadata := getSpecMetadata(rollout) |
There was a problem hiding this comment.
You may want to get this change sooner since results in panic for existing rollout objects :)
natasha41575
force-pushed
the
rollouts/reposync
branch
from
874649c
to
418c0ba
Compare
natasha41575
force-pushed
the
rollouts/reposync
branch
3 times, most recently
from
7d43e2b
to
34e1535
Compare
| } | ||
| _, err = client.Resource(rootSyncGVR).Namespace(rootSyncNamespace).Patch(ctx, name, types.ApplyPatchType, data, metav1.PatchOptions{FieldManager: name}) | ||
| _, err = client.Resource(gvr).Namespace(namespace).Patch(ctx, name, types.ApplyPatchType, data, metav1.PatchOptions{FieldManager: name}) |
There was a problem hiding this comment.
Note for us (since this came up during implementation of metadata on RSync objects): Since this is a patch call, this explains why we don't overwrite the Remote RSync objects.
I was wondering if we can get the |
natasha41575
force-pushed
the
rollouts/reposync
branch
from
34e1535
to
5ac32a8
Compare
natasha41575
force-pushed
the
rollouts/reposync
branch
from
5ac32a8
to
0e9d43c
Compare
natasha41575
force-pushed
the
rollouts/reposync
branch
from
0e9d43c
to
03caa7b
Compare
Fixes #3838.
#3858 contains name changes to make the function of
RemoteRootSyncobject more obvious.This PR takes the approach of reusing the
RemoteRootSynccontroller to also support RepoSync objects (and renames the controller toRemoteSync.Alternatives:
RemoteRepoSynccontroller. I played around with this option for a little bit but it was a lot of duplicated code. Seems better to have it all in one code path. I briefly looked into seeing of go generics + a common library could help with deduping with this option but I didn't immediately see a solution there that was better than reusing the controller.