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 kubectl workspace plugin and adjust usage output #1901

Merged
merged 3 commits into from
Sep 30, 2022

Conversation

hardys
Copy link

@hardys hardys commented Sep 6, 2022

Summary

Elsewhere we describe aliases of ws/workspace/workspaces but the
plugin only works with ws/workspaces atm - this makes it consistent
and allows us to share the same usage with the underlying kcp
plugin subcommand.

Related issue(s)

Discussed in #687 and may enable some clearer and/or more consistent docs, but doesn't actually fix that issue

@hardys hardys changed the title Add kubectl workspace plugin and adjust usage outpu Add kubectl workspace plugin and adjust usage output Sep 6, 2022
@ncdc
Copy link
Member

ncdc commented Sep 6, 2022

Just FYI, I have a ton of plugin structural changes in #1701, in case you're interested.

@hardys
Copy link
Author

hardys commented Sep 6, 2022

Just FYI, I have a ton of plugin structural changes in #1701, in case you're interested.

Ah thanks, will review! 👍

@hardys hardys force-pushed the cli_workspace_plugin branch 2 times, most recently from 4b92a40 to 57cbcfe Compare September 7, 2022 10:52
@hardys
Copy link
Author

hardys commented Sep 7, 2022

I reviewed #1701 and tried it locally - lgtm but doesn't address the issues covered in the PR so I rebased this and adjusted the workspace plugin part to use symlinks instead of further duplicated code (this also removes the current duplication between kubectl-ws and kubectl-workspaces)

@hardys hardys force-pushed the cli_workspace_plugin branch from 57cbcfe to 8902b47 Compare September 7, 2022 14:51
@ncdc ncdc changed the title Add kubectl workspace plugin and adjust usage output 🌱 Add kubectl workspace plugin and adjust usage output Sep 9, 2022
@ncdc ncdc changed the title 🌱 Add kubectl workspace plugin and adjust usage output ✨ Add kubectl workspace plugin and adjust usage output Sep 9, 2022
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2022
@hardys hardys force-pushed the cli_workspace_plugin branch from 8902b47 to f83c8c0 Compare September 23, 2022 10:47
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2022
@hardys hardys force-pushed the cli_workspace_plugin branch from f83c8c0 to c0b429b Compare September 23, 2022 10:49
@hardys
Copy link
Author

hardys commented Sep 23, 2022

Ready for another review, previous comments addressed - thanks for the feedback!

One thing to mention - this only symlinks in the install Makefile target - this means we won't have the CLI aliases in the container image, because the Dockerfile uses the build target then copies kubectl-*

Do we want/need the ws/workspaces aliases in the Dockerfile (or build target) to resolve that?

@hardys hardys requested review from ncdc and sttts September 23, 2022 11:01
@hardys
Copy link
Author

hardys commented Sep 23, 2022

/retest

@ncdc
Copy link
Member

ncdc commented Sep 23, 2022

goreleaser config needs updating

@ncdc
Copy link
Member

ncdc commented Sep 23, 2022

Re the Dockerfile, we should probably ensure the symlinks get into the image. Would it be possible to have them created in the build target?

@hardys hardys force-pushed the cli_workspace_plugin branch from c0b429b to 12ae966 Compare September 26, 2022 12:32
@hardys
Copy link
Author

hardys commented Sep 26, 2022

Re the Dockerfile, we should probably ensure the symlinks get into the image. Would it be possible to have them created in the build target?

Yes this seems like the best option given that COPY resolves any symlinks created by the build target in the makefile - now updated

@hardys
Copy link
Author

hardys commented Sep 26, 2022

goreleaser config needs updating

Ack now updated to reflect the new binary naming, although looking at the docs it seems like we can only create symlinks when generating OS packages - I'll double check this though as I'm not very familiar with goreleaser.

@hardys hardys force-pushed the cli_workspace_plugin branch from c6d4c59 to f033a63 Compare September 26, 2022 14:53
@hardys
Copy link
Author

hardys commented Sep 26, 2022

@ncdc I updated the goreleaser config - AFAICS to generate symlinks in the archive I need to create them in a post build hook, then copy the resulting files, this seems to work running locally but happy to hear if there is a cleaner way :)

$ tar -tvf dist/kubectl-kcp-plugin_0.1.0-SNAPSHOT-c6d4c591_linux_amd64.tar.gz
lrwxrwxrwx shardy/shardy     0 2022-09-26 15:50 bin/kubectl-workspaces -> kubectl-workspace
lrwxrwxrwx shardy/shardy     0 2022-09-26 15:50 bin/kubectl-ws -> kubectl-workspace
-rwxrwxr-x shardy/shardy 94323912 2022-09-26 15:50 bin/kubectl-workspace
-rwxrwxr-x shardy/shardy 101777416 2022-09-26 15:50 bin/kubectl-kcp

@hardys hardys force-pushed the cli_workspace_plugin branch from f033a63 to 388962a Compare September 27, 2022 15:38
Steven Hardy added 3 commits September 29, 2022 16:18
Elsewhere we describe aliases of ws/workspace/workspaces but the
plugin only works with ws/workspaces atm - this makes it consistent,
removes the duplicate kubectl-ws code, and and allows us to share
the same usage with the underlying kcp plugin subcommand.
The plugin can be called via "kubectl kcp workspaces" or since kcp-dev#881 also via
"kubectl workspaces" - when calling in the latter context it's
confusing to get usage info referring to the kcp plugin.
@hardys hardys force-pushed the cli_workspace_plugin branch from 388962a to 4eef8f5 Compare September 29, 2022 15:19
@hardys
Copy link
Author

hardys commented Sep 30, 2022

@ncdc this is ready for another review pass when you get a moment, thanks!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 30, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc

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 Sep 30, 2022
@openshift-merge-robot openshift-merge-robot merged commit b1d3600 into kcp-dev:main Sep 30, 2022
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.

5 participants