Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

fleetctl: destroy using wildcards can search in the repository #1256

Closed
wants to merge 1 commit into from

Conversation

wuqixuan
Copy link
Contributor

Curerntly, fleetctl destroy *.service, it will search the units
in the local directory. The args with wildcards have already parsed
by golang package before be passed into runDestroyUnits. So
runDestroyUnits cannot do anything.

This patch support wildcards just search in the repository,
not from local directory. But need support a new interface. The new
interface is:
fleetctl destroy "xx*.service".
We found that if bracket the service name, the arg will not be
parsed by golang package to local directory. Then we can do match it
with the service name in the repository.

Fixes #710

Curerntly, fleetctl destroy *.service, it will search the units
in the local directory. The args with wildcards have already parsed
by golang package before be passed into runDestroyUnits. So
runDestroyUnits cannot do anything.

This patch support wildcards just search in the repository,
not from local directory. But need support a new interface. The new
interface is:
    fleetctl destroy "xx*.service".
We found that if bracket the service name, the arg will not be
parsed by golang package to local directory. Then we can do match it
with the service name in the repository.

Fixes coreos#710
@bcwaldon
Copy link
Contributor

bcwaldon commented Jul 9, 2015

I don't want to support this just for fleetctl destroy, and the existing behavior needs to be preserved. I have two requests:

  1. can we escape an asterisk with a backslash or something to prevent bash from rendering it before it gets into golang code?
  2. could you make sure this works for all fleetctl commands that make sense?

@wuqixuan
Copy link
Contributor Author

@bcwaldon

  1. One wrong information is in my previous comment. Its the bash who search in the local directory using * character, not golang. So prevent the convert is my solution of the patch. I think using backslash maybe can get the same result. But backslash and double quotation marks are different interface. So which one do you think is better? Currently I cannot find the third one.
  2. Destroy command and others command are different. Because it only handle the unit on the registry. While submit always handle units on the local directory. Load and start command can handle units on both places. So its difficult to decide which one need the same feature. Let me think over. And what's your opinion ?

@antrik
Copy link
Contributor

antrik commented Mar 14, 2016

Let's try to sum up the current situation:

  1. This shouldn't be a surprise to anyone -- but just to avoid any possible confusion: there is nothing fleetctl can do to disable the shell's file globbing. It's on the user to quote the parameters appropriately for the shell. This patch doesn't really have anything to do with this. (The description is rather confusing in this regard...)
  2. This patch needs major rework for upstream changes that happened in fleetctl.
  3. Regarding other commands: as I understand it, "unload" and "stop" should be straightforward to support in the same manner as "destroy". "load" and "start" need some more consideration; but I think it should also be doable, by adding the globbing in the appropriate branches of the existing file/registry case handling. "status" is probably also more or less straightforward. Among the commands operating on a list of units provided on the command line, I guess "submit" is the only one where globbing doesn't come in at all.

(For the commands operating on a single unit -- such as "cat", "journal" etc. -- it would be possible in principle to support globbing as well, as long as only a single unit is actually matched. The benefit of that would be rather disputable though -- I'd tend to skip these.)

  1. The way this is implemented right now, it would introduce new performance problems, and prevent fixing existing problems in some paths. (See fleetctl: optimize stop/unload commands #1344 for example.)

I guess the impact could be mostly mitigated by trying to check up front whether any of the arguments supplied looks like a glob pattern. That would result in a gotcha though: don't use globbing if you have a lot of services...

@antrik
Copy link
Contributor

antrik commented Mar 15, 2016

Oh, one more thing I forgot to mention: obviously this needs a bunch of new test cases on top of everything else.

@jonboulle
Copy link
Contributor

In the absence of more people clamouring for this feature I am kind of leaning towards just closing this as wontfix. I don't see a really strong use case for it and something essentially equivalent can be easily mimicked with a shell one-liner. @antrik what do you think?

@antrik
Copy link
Contributor

antrik commented Mar 16, 2016

@jonboulle well, as I'm not actually a fleet user, I only have a vague idea of how much value it would add... I tend to agree though that it doesn't look really important. If someone else feels like picking it up, I wouldn't mind getting it in; but I guess we shouldn't spend our resources on this.

@jonboulle
Copy link
Contributor

Let's revisit if there's further demand.

@jonboulle jonboulle closed this Mar 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants