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.

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

✨ Add bind workload cmd #2258

Merged
merged 3 commits into from
Oct 31, 2022
Merged

Conversation

qiujian16
Copy link
Contributor

@qiujian16 qiujian16 commented Oct 25, 2022

Signed-off-by: Jian Qiu jqiu@redhat.com

Summary

Add a bind compute command to create placement to use locations in the compute workspace

  • kubectl kcp bind compute <my location workspace> creates a placement, an apibinding to global kubernetes APIExport and an apibinding to kubernetes APIExport in compute workspace if at least one synctarget supports these APIExports.
  • kubectl kcp bind compute <my location workspace>--apiexports root:ws1:foo.bar creates a placement, an apibinding to foo.bar APIExport if at least one synctarget supports the APIExports.
  • --location-selectos and --namespace-selector is to set placement, selectAll is set if not specified.

Related issue(s)

Fixes #2102

@davidfestal
Copy link
Member

@qiujian16 Do you plan to create a distinct PR for the following part mentioned at the end of the related issue description:

In this task we should also remove the defaultplacement_controller.go, since this command would replace it.

?

@davidfestal davidfestal self-requested a review October 25, 2022 09:14
@qiujian16
Copy link
Contributor Author

@qiujian16 Do you plan to create a distinct PR for the following part mentioned at the end of the related issue description:

In this task we should also remove the defaultplacement_controller.go, since this command would replace it.

?

The plan is to include it in this PR. I haven't finished that yet, because I need to create some helper func in e2e test for this bind compute at first to avoid breaking e2e.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2022
Copy link
Member

@davidfestal davidfestal left a comment

Choose a reason for hiding this comment

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

In addition to individual review comments, I'm not very comfortable with the

kcp bind workload ...

syntax.

I'd rather have:

kcp bind compute

as initially proposed in the description, or

kcp bind location

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2022
@qiujian16 qiujian16 changed the title ✨ [WIP] Add bind workload cmd ✨ Add bind workload cmd Oct 31, 2022
Signed-off-by: Jian Qiu <jqiu@redhat.com>
Signed-off-by: Jian Qiu <jqiu@redhat.com>
@qiujian16 qiujian16 force-pushed the bind-compute branch 3 times, most recently from 5817e73 to 8059d7f Compare October 31, 2022 10:18
@qiujian16 qiujian16 force-pushed the bind-compute branch 2 times, most recently from 2d39eab to e4a2e46 Compare October 31, 2022 13:33
Signed-off-by: Jian Qiu <jqiu@redhat.com>
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 31, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidfestal

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2022
@openshift-merge-robot openshift-merge-robot merged commit 0c43abf into kcp-dev:main Oct 31, 2022
@qiujian16 qiujian16 deleted the bind-compute branch November 1, 2022 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement a CLI command to bind a user workspace to some compute.
4 participants