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 sort-manifests plugin #144

Merged
merged 1 commit into from May 9, 2019

Conversation

Projects
None yet
4 participants
@superbrothers
Copy link
Contributor

commented May 1, 2019

This PR adds a new plugin manifest "sort-by-kind".

When installing manifests, they should be sorted in a proper order by Kind. For example, Namespace object must be in the first place when installing them.

"sort-by-kind" plugin sorts manfest files in a proper order by Kind, which is implementd by using tiller.SortByKind() in Kubernetes Helm.

$ ls ./manifests
deployment.yaml  ingress.yaml  namespace.yaml  service.yaml
$ kubectl sort-by-kind ./manifests | kubectl apply -f -

Checklist for plugin developers:

  • Read the Plugin Naming Guide (for new plugins)
  • Verify the installation from URL or a local archive works (kubectl krew install --manifest=[...] --archive=[...])
@corneliusweig

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

I find the name not very intuitive. Based on what it does, I find sort-for-install or something alike more descriptive.

@corneliusweig

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

Thinking further, this version only supports install order. Why not also support

  • Uninstall order
  • alphabetically by resource name
  • alphabetically by resource kind
  • and maybe more?
    Then the command name could be even more expressive, like sort-manifests with subcommands for-apply, for-delete by-name, by-kind,...
@ahmetb

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

I don't use Helm, but it sounds to me like alphabetical order (based on filename) isn't important to people (as kubectl apply would already do it).

Similarly, uninstall order isn't as important, you can delete a namespace and then a subresource in it, and kubectl delete would already attempt to delete all, at the end it would succeed. But maybe I'm missing something.

Thanks for reviewing @corneliusweig! Let me know if you have any interest in being a co-maintainer of index repo.

@superbrothers

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

I find sort-for-install or something alike more descriptive.

This plugin command also supports uninstall order:

$ kubectl sort-by-kind manifests/ --delete | kubectl delete -f -
@ahmetb

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

I think a cleaner design would be:

kubectl sort-manifests --by=[kind,name,...] [--for-delete or --uninstall]

sort-by-kind is too specific. For a "plugin name", sort-manifests, manifest-sort, or even sort is better.

@superbrothers

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Isn't sort too generic name? At first I intended to be sort, but I thought it was too general and I chose sort-by-kind. If there are no problem, I would like to choice sort.

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

Yes “sort” is a bit too generic, although I can’t think of anything else to be sorted with kubectl. I recomment “sort-manifests”

@superbrothers

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Thank you for suggestion. I renamed this plugin to "sort-manifests" at f3a3d95.

@corneliusweig

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

Let me know if you have any interest in being a co-maintainer of index repo.

@ahmetb I'd be happy to help.

I think the plugin name sort-manifests is good.

@superbrothers Kustomize implements a very similar sort condition in sigs.k8s.io/kustomize/pkg/resmap/idslice.go. Depending on this for sorting instead of tiller could reduce the file size of your binary.
Btw, any particular reason why you didn't provide a windows build in your manifest?

@superbrothers

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

@corneliusweig

Kustomize implements a very similar sort condition in sigs.k8s.io/kustomize/pkg/resmap/idslice.go.

This looks good. I'll try to use it in the future.

Btw, any particular reason why you didn't provide a windows build in your manifest?

Because I don't have a Windows machine, so I can't support it.

@ahmetb ahmetb changed the title Add sort-by-kind plugin manifest Add sort-manifests plugin May 7, 2019

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Looks good to me, one minor nit.
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm and removed lgtm labels May 7, 2019

@superbrothers superbrothers force-pushed the superbrothers:sort-by-kind-v0.1.0 branch from 07951eb to d76c616 May 9, 2019

@superbrothers

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

I squashed commits.

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label May 9, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link

commented May 9, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, superbrothers

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

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

🤔 the manifest validator is failing for some reason.

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

/hold

@superbrothers

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

The command "env GOBIN=$HOME/bin go get github.com/GoogleContainerTools/krew/cmd/validate-krew-manifest" failed and exited with 2 during .

@ahmetb Could you retry the CI build? 🤔

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

#145 should fix it.
/hold cancel

@ahmetb ahmetb merged commit 32b3d15 into kubernetes-sigs:master May 9, 2019

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
tide Not mergeable. Should not have do-not-merge/hold label.
Details
cla/linuxfoundation superbrothers authorized
Details

@superbrothers superbrothers deleted the superbrothers:sort-by-kind-v0.1.0 branch May 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.