Skip to content
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

validate: increase os verification to checklinux #366

Closed
wants to merge 1 commit into from

Conversation

zhouhao3
Copy link

If os is not linux, then do not have to implement the next code, improve efficiency.

Signed-off-by: zhouhao zhouhao@cn.fujitsu.com

Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>
@Mashimiao
Copy link

OS limitation we have checked in CheckOS() before, here is not necessary.

@zhouhao3
Copy link
Author

OS limitation we have checked in CheckOS() before, here is not necessary.

If the error in the checkos, will not immediately return to the error, but also continue to execute checklinux, so the implementation of checklinux those excess code, so I think it should increase this judgment.

@Mashimiao
Copy link

Plaform has been removed from spec, so closing this.

@Mashimiao Mashimiao closed this Jul 5, 2017
@zhouhao3
Copy link
Author

zhouhao3 commented Jul 6, 2017

@Mashimiao I think only need rebase on it. This judgment is still needed, as I said above, through this judgment can reduce the number of unnecessary functions to run.

@Mashimiao Mashimiao reopened this Jul 6, 2017
@Mashimiao
Copy link

reopened, but I think this fix is not suitable. It will be better if we can add judge if target platform is linux to work with v.spec.Linux != nil

@wking
Copy link
Contributor

wking commented Jul 10, 2017 via email

@zhouhao3
Copy link
Author

I closing the PR, because of @wking's advice and the change of #416.

@zhouhao3 zhouhao3 closed this Jul 11, 2017
@zhouhao3 zhouhao3 deleted the linux-check branch October 18, 2017 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants