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

fleetctl: print error when units were not found in registry #1503

Merged
merged 1 commit into from
Jul 8, 2016

Conversation

kayrus
Copy link
Contributor

@kayrus kayrus commented Mar 14, 2016

Destroy logic was already improved so this issue could be closed: #1256
#710

But we still have to print some kind of error message when units were not found.

@tixxdz
Copy link
Contributor

tixxdz commented Mar 14, 2016

Hi @kayrus thank you for the patch! but since we agreed on ignoring non-existent units and don't fail we added unit tests for that case, and the patch just triggers those failures., and also findUnits() filters units based on what you have provided, if you did provided non-existent units, it will return an empty list and not an error. The callers assume an empty list if units do not exist, errors are only in case the Units() fail.

@kayrus kayrus force-pushed the kayrus/units_not_found branch 2 times, most recently from 6516506 to 39e0bfc Compare March 14, 2016 10:51
@kayrus
Copy link
Contributor Author

kayrus commented Mar 14, 2016

@tixxdz updated

@tixxdz
Copy link
Contributor

tixxdz commented Mar 14, 2016

ok thank you could you also please fix gofmt and push again ;-)

While you are it why is this related to #1256 #710 ?? am I missing something here ? when you call with wildcards! I guess it's has nothing to do with fleetctl command or Go, it's you shell which expands automatically and by accident to local valid unit services..., you should know that you are doing or escape ?! are these really bugs related to fleet ? hmm ?

@kayrus kayrus changed the title fleetctl: return error when units were not found in registry fleetctl: print error when units were not found in registry Mar 14, 2016
@kayrus
Copy link
Contributor Author

kayrus commented Mar 14, 2016

Btw, should we filter units when we would like to print only one unit? I.e.

$ systemctl list-unit-files etcd2.service
UNIT FILE     STATE   
etcd2.service disabled

1 unit files listed.
$ systemctl list-units etcd2.service
UNIT          LOAD   ACTIVE SUB     DESCRIPTION
etcd2.service loaded active running etcd2

LOAD   = Reflects whether the unit definition was properly loaded.
ACTIVE = The high-level unit activation state, i.e. generalization of SUB.
SUB    = The low-level unit activation state, values depend on unit type.

1 loaded units listed. Pass --all to see loaded but inactive units, too.
To show all installed unit files use 'systemctl list-unit-files'.
$ fleetctl list-unit-files hello.service
UNIT                    HASH    DSTATE          STATE           TARGET
hello.service           3750b3e launched        launched        706e4d01.../coreos1
hello@1.service         79699e3 launched        launched        706e4d01.../coreos1
hello@2.service         79699e3 launched        launched        706e4d01.../coreos1
hello@3.service         79699e3 launched        launched        706e4d01.../coreos1
$ fleetctl list-units hello.service
UNIT                    MACHINE                 ACTIVE  SUB
hello.service           706e4d01.../coreos1     active  running
hello@1.service         706e4d01.../coreos1     active  running
hello@2.service         706e4d01.../coreos1     active  running
hello@3.service         706e4d01.../coreos1     active  running

@tixxdz
Copy link
Contributor

tixxdz commented Mar 14, 2016

@kayrus hmm IMO yes it make sense, not sure what others think

I guess the only thing to worry about is the exit codes, since list-unit-files always succeeded, this should not change. If unit tests are not there then this should also be added.

@antrik
Copy link
Contributor

antrik commented Mar 14, 2016

I'm not convinced of this change: it seems a bit arbitrary to me that it warns if no units were matched, but not if some of the supplied units were matched while others were not.

At the very least, the warning should be clearer, e.g. "Warning: No units matched."

I'm not sure I understand the remark about listing one unit. (Isn't that a separate issue?...)

@dongsupark
Copy link
Contributor

LGTM.
I think there's no reason not to merge this. I'll merge it tomorrow.

@dongsupark dongsupark merged commit f66a95a into coreos:master Jul 8, 2016
dongsupark pushed a commit that referenced this pull request Jul 8, 2016
fleetctl: print error when units were not found in registry
dongsupark pushed a commit that referenced this pull request Jul 8, 2016
fleetctl: print error when units were not found in registry
dongsupark pushed a commit that referenced this pull request Jul 8, 2016
fleetctl: print error when units were not found in registry
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants