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

fleetctl: check for empty input unit strings #1461

Closed
dongsupark wants to merge 4 commits into from
Closed

fleetctl: check for empty input unit strings #1461

dongsupark wants to merge 4 commits into from

Conversation

@dongsupark
Copy link

@dongsupark dongsupark commented Mar 3, 2016

If no unit name is given for fleetctl commands like destroy, load,
start, stop, submit, and unload, then return with exit code 0,
printing out a soft warning message to stderr.

Fixes: #1443

/cc @kayrus @tixxdz @jonboulle

@kayrus
Copy link
Contributor

kayrus commented Mar 3, 2016

@dongsupark can you put these checks somewhere in one place. these copy-pastes look terrible.

@dongsupark dongsupark force-pushed the dongsu/error-empty-units branch from 44272de to 92f287a Compare March 4, 2016 10:27
@dongsupark
Copy link
Author

dongsupark commented Mar 4, 2016

@kayrus I updated the branch dongsu/error-empty-units.
I wrote a simple wrapper in pkg/args.go, which can be called from each fleetctl command.

@tixxdz
Copy link
Contributor

tixxdz commented Mar 4, 2016

@jonboulle lgtm

@jonboulle
Copy link
Contributor

jonboulle commented Mar 4, 2016

uh, this helper is way overkill. please just revert to the inline check...

@jonboulle
Copy link
Contributor

jonboulle commented Mar 4, 2016 

    if pkg.IsArgsEmpty(args) {
        stderr("No units given")
        return 0
    }

is no simpler than

    if len(args) == 0 {
        stderr("No units given")
        return 0
    }

@kayrus
Copy link
Contributor

kayrus commented Mar 4, 2016

@jonboulle is it possible to check arguments inside init function?

@jonboulle
Copy link
Contributor

jonboulle commented Mar 4, 2016

Why do you need to check inside init? In the context where you're checking, you've just been passed them.

@dongsupark dongsupark force-pushed the dongsu/error-empty-units branch from 92f287a to a2d16c6 Compare March 4, 2016 14:16
@dongsupark
Copy link
Author

dongsupark commented Mar 4, 2016

@jonboulle all right. I reverted it to the 1st version. So there's no helper in pkg/args.go.

@jonboulle
Copy link
Contributor

jonboulle commented Mar 4, 2016

Thanks! Could you add a test for this? :-)

@dongsupark
Copy link
Author

dongsupark commented Mar 4, 2016

@jonboulle Sure, I can.
Actually a PR by @tixxdz #1460 already contains a set of necessary tests for start and load. I would rather wait for the PR to be first merged, and add my tests on top of that.

Djalal Harouni added 2 commits March 4, 2016 16:46
fleetctl:test: make newFakeRegistryForCommands() able to handle templ…
e22a024 
…ate units

This is a preparation patch for the next start test patch that will try
to create and start units from a template unit
fleetctl:test: improve the start test and make sure that it handles t…
e492091 
…emplates

Since we create units from a template unit, there is a real wait
operation. To avoid that and be able to really test functions, just make
template units global by default, later new units will be global and we
won't wait forever to reach the targetstate we just assume the
Desiredstate.
jonboulle
jonboulle reviewed Mar 4, 2016
View reviewed changes
fleetctl/destroy.go Outdated
}

func runDestroyUnits(args []string) (exit int) {
// Even if no unit is given by user, it should not exit with error,
Copy link
Contributor

@jonboulle jonboulle Mar 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the comment is necessary. Please just remove it here and everywhere else.

Dongsu Park added 2 commits March 4, 2016 17:35
fleetctl: check for empty input unit strings
e6e8499 
If no unit name is given for fleetctl commands like destroy, load,
start, stop, submit, and unload, then return with exit code 0,
printing out a soft warning message to stderr.

Fixes: coreos#1443
fleetctl:test: check for empty input
e613e8c 
Check that the return value is 0, when the following commands run
without any input unit names: destroy, load, start, stop, and unload.
@dongsupark dongsupark mentioned this pull request Mar 7, 2016
Merged
@dongsupark dongsupark force-pushed the dongsu/error-empty-units branch from a2d16c6 to e613e8c Compare March 7, 2016 13:58
@dongsupark
Copy link
Author

dongsupark commented Mar 7, 2016

Updated the tree as @jonboulle suggested.
I erase the comments as suggested.
I also added simple tests for destroy, load, start, stop, and unload.
Submit test is missing in this PR, because there's no fleetctl/submit_test.go.
So I created a new test submit_test.go that is covered in #1473.

@tixxdz
Copy link
Contributor

tixxdz commented Mar 7, 2016

@dongsupark LGTM, but could you please create a new branch then force push on top of this so semaphore could run again ?

@dongsupark dongsupark closed this Mar 7, 2016
@dongsupark dongsupark deleted the dongsu/error-empty-units branch March 7, 2016 14:40
@dongsupark dongsupark restored the dongsu/error-empty-units branch March 7, 2016 14:41
@dongsupark dongsupark deleted the dongsu/error-empty-units branch March 7, 2016 14:42
@dongsupark dongsupark mentioned this pull request Mar 7, 2016
Merged
@dongsupark
Copy link
Author

dongsupark commented Mar 7, 2016

Closed in favor of #1475

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dongsupark @kayrus @tixxdz @jonboulle