Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🌱 Syncer: Use the features now provided by the Syncer VW, based on transformations #2321

Closed
wants to merge 1 commit into from
Closed

🌱 Syncer: Use the features now provided by the Syncer VW, based on transformations #2321

wants to merge 1 commit into from

Conversation

davidfestal
Copy link
Member

Summary

Syncer: Fix TODOs related to transformations: let's use the features now provided by the Syncer VW, based on transformations, instead of in-syncer implementation.

Related issue(s)

kcp-dev/contrib-tmc#103

@openshift-ci openshift-ci bot requested a review from csams November 7, 2022 20:49
@openshift-ci openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Nov 7, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 7, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sttts for approval by writing /assign @sttts in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@davidfestal
Syncer: Fix TODOs related to transformations...
473dcd3 
... Let's use the features now provided by the
Syncer VW based on transformations, instead of
in-syncer implementation.

Signed-off-by: David Festal
@davidfestal davidfestal force-pushed the adapt-syncer-to-syncer-transformations branch from 1a32070 to 473dcd3 Compare November 8, 2022 12:36
@jmprusi
Copy link
Member

jmprusi commented Nov 9, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 9, 2022
@davidfestal
Copy link
Member Author

/assign @sttts

@davidfestal davidfestal added this to the v0.10 milestone Nov 10, 2022
@davidfestal davidfestal added the area/transparent-multi-cluster Related to scheduling of workloads into pclusters. label Nov 10, 2022
@openshift-merge-robot
Copy link
Contributor

@davidfestal: PR needs rebase.

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-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2022
stevekuznetsov
stevekuznetsov reviewed Nov 15, 2022
View reviewed changes
hack/logcheck.out
@@ -33,8 +33,8 @@
/pkg/admission/webhook/generic_webhook.go:142:5: function "Errorf" should not be used, convert to contextual logging
/pkg/admission/webhook/generic_webhook.go:172:4: function "Errorf" should not be used, convert to contextual logging
/pkg/authorization/delegated/authorizer.go:45:3: function "Errorf" should not be used, convert to contextual logging
/pkg/cliplugins/workload/plugin/sync.go:591:4: function "Infof" should not be used, convert to contextual logging
/pkg/cliplugins/workload/plugin/sync.go:591:4: function "V" should not be used, convert to contextual logging
/pkg/cliplugins/workload/plugin/sync.go:592:4: function "Infof" should not be used, convert to contextual logging
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use contextual logging?

pkg/syncer/shared/finalizer.go
@@ -73,31 +72,26 @@ func EnsureUpstreamFinalizerRemoved(ctx context.Context, gvr schema.GroupVersion
}
upstreamObj.SetFinalizers(desiredFinalizers)

// TODO(jmprusi): This code block will be handled by the syncer virtual workspace, so we can remove it once
// the virtual workspace syncer is integrated
// TODO(davidfestal): The following code block should be removed as soon as the Advanced Scheduling feature is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe you all have no problem remembering all of your TODOs but I would suggest making this code panic or not compile or something to make it easier to find what you need to change when scheduling is removed :)

pkg/syncer/shared/finalizer.go
} else if err == nil {
logger.V(2).Info("Updated upstream resource to remove the syncer finalizer")
} else {
logger.V(3).Info("Didn't update upstream resource to remove the syncer finalizer, since the upstream resource doesn't exist anymore")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err != nil here, we likely want to log it, right?

@davidfestal davidfestal modified the milestones: v0.10, v0.11 Dec 2, 2022
@ncdc
Copy link
Member

ncdc commented Feb 22, 2023

@davidfestal do you plan to proceed with this?

@ncdc
Copy link
Member

ncdc commented Feb 22, 2023

/milestone clear

@openshift-ci openshift-ci bot removed this from the v0.11 milestone Feb 22, 2023
@davidfestal
Copy link
Member Author

/hold until the kcp-glbc is moved out from using Advanced scheduling to use the coordination controller pattern.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 14, 2023
@xrstf
Copy link
Contributor

xrstf commented Jun 6, 2023

With Syncer/TMC having been removed, I think we can close this PR.

@kcp-ci-bot
Copy link
Contributor

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

@kcp-ci-bot kcp-ci-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 23, 2023
@mjudeikis mjudeikis closed this Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevekuznetsov stevekuznetsov stevekuznetsov left review comments

@jmprusi jmprusi Awaiting requested review from jmprusi

@csams csams Awaiting requested review from csams

Assignees

@sttts sttts

@jmprusi jmprusi

Labels
area/transparent-multi-cluster Related to scheduling of workloads into pclusters. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
kcp
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@davidfestal @jmprusi @openshift-merge-robot @ncdc @xrstf @kcp-ci-bot @stevekuznetsov @sttts @mjudeikis