New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add test cases #52
add test cases #52
Conversation
06c98a1
to
c0464e6
Compare
d8252cf
to
c57f09d
Compare
I have tried to run e2e tests in travis but fails because of the time limit. |
PTAL @Superxi911 |
"github.com/caicloud/cyclone/store" | ||
"github.com/emicklei/go-restful" | ||
docker_client "github.com/fsouza/go-dockerclient" | ||
) | ||
|
||
const WORKER_NODE_DOCKER_VERSION = "1.10.1" | ||
// WORKER_NODE_DOCKER_VERSION defines the required docker version in worker node. | ||
const WORKER_NODE_DOCKER_VERSION = "WORKER_NODE_DOCKER_VERSION" |
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.
I think we should restrict it by code, not by ENV
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.
For example, if we wanna change the supported docker version in worker node, we have to re-compile the code.
It's not cool.
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.
If we wanna change the supported docker version in worker node in future, it means we have update our codes to support more docker version, not just modify a const string.
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.
I changed here because travis's docker version is 1.12.0, and this version could pass e2e tests.
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.
And now the default is 1.10.1, I think it would be OK.
We could add some tips in doc, for example "This is just for test."
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.
Ok, suggest user don't configure this ENV in doc.
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.
OK
Client *docker_client.Client | ||
Registry string | ||
AuthConfig *AuthConfig | ||
EndPoint string |
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.
Why expose these vars? We have functions to get them, such as: GetDockerAuthConfig, GetDockerRegistry. If you expose them, them can be modified
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.
just for test, we can not mock this manager if we don't expose them.
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.
Would exposed the vars violate the golang's coding style? if yes, i think can delete the functions, as GetDocker***
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.
OK
Signed-off-by: Ce Gao <ce.gao@outlook.com>
c57f09d
to
41ee996
Compare
PTAL |
LGTM |
fixes #43
Signed-off-by: Ce Gao ce.gao@outlook.com