Skip to content
This repository has been archived by the owner on May 3, 2022. It is now read-only.

ref(duffle): hide http-based workflow commands (pull, push, search) #491

Merged
merged 1 commit into from
Nov 21, 2018
Merged

ref(duffle): hide http-based workflow commands (pull, push, search) #491

merged 1 commit into from
Nov 21, 2018

Conversation

bacongobbler
Copy link
Contributor

@bacongobbler bacongobbler commented Nov 21, 2018

While we are still sorting out the server-side part of the specification, we should remove the CLI part of the codebase. This change means that the only way to install a bundle from a URL is through the duffle install --insecure -f flag.

closes #317
closes #418
closes #426

TODO:

  • remove docs mentioning duffle pull, duffle push and duffle search.

Signed-off-by: Matthew Fisher matt.fisher@microsoft.com

@bacongobbler bacongobbler changed the title ref(duffle): remove http-based workflow commands ref(duffle): remove http-based workflow commands (pull, push, search) Nov 21, 2018
@michelleN
Copy link
Contributor

Hey @bacongobbler - I see you're removing push/pull/search here. I know we decided that we probably don't want people to get confused at launch with the repo/server side functionality but I hate to lose this work.

What do you think about just making those commands hidden? If we add a Hidden: true attribute to the cobra command (like below), it won't show up in the list of commands, but if someone knows it's there, they can still use it and we can still play around and iterate that functionality.

	cmd := &cobra.Command{
		Use:    "search",
		Short:  "perform a fuzzy search on available bundles",
		Hidden: true,

@bacongobbler
Copy link
Contributor Author

bacongobbler commented Nov 21, 2018

IMO at this point I believe it's best if the commands were all thrown away and to start from scratch, starting with proper repository client packages and a backend that conforms to the repository API laid out in the CNAB repository specification. The current pull/push/search API does not conform at all to the spec, relying on old assumptions we made during initial proof of concepts, is buggy and (mostly) un-salvagable code. I'm considering shutting off hub.cnlabs.io for the time being (because of the above reasons RE: non-conforming/buggy), so there would be no backend to use anyways.

I'm open to the idea if you think there's parts of push/pull/search that should be salvaged, but from my point of view it doesn't look like any of it can be salvaged/refactored.

@bacongobbler
Copy link
Contributor Author

Actually I found a case where we may want to retain it for future use... the duffle search --output flag and respective tests @vdice added in #483. I'll add those back and just focus on refactoring duffle install.

@bacongobbler bacongobbler changed the title ref(duffle): remove http-based workflow commands (pull, push, search) ref(duffle): hide http-based workflow commands (pull, push, search) Nov 21, 2018
@bacongobbler
Copy link
Contributor Author

okay, I hid the commands from output but retained their existing behaviour.

@vdice
Copy link
Member

vdice commented Nov 21, 2018

Actually I found a case where we may want to retain it for future use... the duffle search --output flag and respective tests @vdice added in #483. I'll add those back and just focus on refactoring duffle install.

Indeed, @bacongobbler , I was going to put up a functional test PR in this repo wherein the default would be to iterate over all remote bundles (via duffle search) and run basic duffle commands against each -- however, not all remote bundles appear ready for primetime (duffle install fails on some, etc.), so I'm running tests against just one known working bundle for now (aks.0.2.0). All of which is to say, I do think the value of somehow having a bot query for all/some remote bundles would be handy -- but it doesn't nec have to be via cli...

@bacongobbler
Copy link
Contributor Author

bacongobbler commented Nov 21, 2018

Yeah agreed. The "remote bundle" story is in limbo at the moment so I wouldn't invest time writing functional tests on that side, at least not until after KubeCon. Once we sort that story out then I absolutely agree that having some test infrastructure around it would be +💯

@vdice
Copy link
Member

vdice commented Nov 21, 2018

LGTM - though I think there still may be references to search (and others mentioned here?) in README.md

@bacongobbler
Copy link
Contributor Author

Good catch. I originally removed those references from the README but then added them back when I hid those CLI commands. I think it'll just confuse users keeping them in. I'll remove them. :)

Signed-off-by: Matthew Fisher <matt.fisher@microsoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants