-
Notifications
You must be signed in to change notification settings - Fork 79
tests: added tests for all commands except run, write, copy, and version #55
Conversation
Addresses most of #24 |
SHOULD we emit an error in this case? There might be something to be said for commands being (optionally?) idempotent? |
I think if you try to remove a mount point, and the mount point doesn't exist, it should at least say something. Perhaps just print a warning to stderr and have a 0 exit code? |
Honestly I'd just prefer to leave it on the user to ignore the error if they're removing something they're not sure would exist. |
I am somewhat receptive to that but then it should be something consistent at the CLI layer (e.g. probably a particular error code) so that people can actually predictably ignore it. /cc @krobertson thoughts? |
TESTS_PATH="${REPO_PATH}/tests" | ||
|
||
export ACBUILD_BIN="${PWD}/bin/acbuild" | ||
export GOPATH=${PWD}/gopath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quote, for consistency
56081c0
to
2c8b05f
Compare
Should be ready for another pass. When acbuild is asked to remove something that doesn't exist the exit code will be 2, so that users can check what type of error was encountered. It'll also be easy going forward to add other error types with different exit codes. |
var ErrNotFound = fmt.Errorf("element to be removed does not exist in this ACI") | ||
|
||
// GetErrorCode will return the recommended exit code for acbuild given an error | ||
func GetErrorCode(err error) int { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting approach. I think this function should exist in the CLI package instead though (but still key off the lib.Err types)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I guess it does make more sense to be there. Fixed.
Should be ready for another pass. |
LGTM. Oddly, github doesn't do syntax highlighting on the "set-user_test.go" files... maybe doesn't like the dash? For the previous point about erroring when removing something not found, general command line behavior is to error. If you do just |
This commit adds test for the following commands: - annotation add - annotation remove - dependency add - dependency remove - end - environment add - environment remove - label add - label remove - mount add - mount remove - port add - port remove - set-exec - set-group - set-name - set-user
tests: added tests for all commands except run, write, copy, and version
An issue was found where remove commands will not emit an error if asked to remove a nonexistent item. This commit fixes that.
Additionally this commit adds test for the following commands: