Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

kata-env: kata-env error out when there is no VERSION_ID. #1178

Merged
merged 1 commit into from
Jan 31, 2019

Conversation

yyyeerbo
Copy link
Contributor

For example, under debian buster/sid. Those information should be
provide with best effort instead of error out.

Fixes: #1177

Signed-off-by: Yang Bo [email protected]

@yyyeerbo
Copy link
Contributor Author

/test

cli/utils.go Outdated
@@ -71,7 +71,7 @@ func getDistroDetails() (name, version string, err error) {
}
}

if name != "" && version != "" {
if name != "" || version != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, if we found one of them we should probably return it.
I suspect it might be good if we set the unfound item to something like <not set> - and maybe do that for both if neither is found, and not error out at all. @jodh-intel - wdyt?

Copy link
Contributor Author

@yyyeerbo yyyeerbo Jan 25, 2019

Choose a reason for hiding this comment

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

we return what we know and what we don't know(set to <not set> or <unknown> looks better, I think. :)

@amshinde
Copy link
Member

/test

@amshinde
Copy link
Member

amshinde commented Jan 25, 2019

lgtm, with returning the not found item as unknown or not-set

Approved with PullApprove

@yyyeerbo
Copy link
Contributor Author

yyyeerbo commented Jan 28, 2019

Update to return "<<unknown>>" when name/version is not defined.

grahamwhaley
grahamwhaley previously approved these changes Jan 28, 2019
Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

thanks for the updates!
lgtm

@grahamwhaley
Copy link
Contributor

/test

@jodh-intel
Copy link
Contributor

jodh-intel commented Jan 28, 2019

Thanks @yyyeerbo. I'm hoping we can overcome this issue (kata-containers/documentation#358 (review)), but I agree this PR is a good idea regardless.

lgtm

Approved with PullApprove

@amshinde
Copy link
Member

/retest

2 similar comments
@jodh-intel
Copy link
Contributor

/retest

@gnawux
Copy link
Member

gnawux commented Jan 31, 2019

/retest

@yyyeerbo
Copy link
Contributor Author

fix bug and testcase..

@gnawux
Copy link
Member

gnawux commented Jan 31, 2019

/test

For example, under debian buster/sid. Those information should be
provide with best effort instead of error out. Set name and version
to "<<unknown>>" if they are not defined.

Fixes: kata-containers#1177

Signed-off-by: Yang Bo <[email protected]>
@yyyeerbo
Copy link
Contributor Author

fix testcases

@caoruidong
Copy link
Member

/test

@jodh-intel
Copy link
Contributor

Restarted failing CIs (metrics timed out).

Copy link
Member

@gnawux gnawux left a comment

Choose a reason for hiding this comment

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

now let's wait the test results

Copy link
Member

@bergwolf bergwolf left a comment

Choose a reason for hiding this comment

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

LGTM

@devimc
Copy link

devimc commented Jan 31, 2019

lgtm

ARM and metrics CIs are not stable, merging...

Approved with PullApprove

@devimc devimc merged commit 5f1ca16 into kata-containers:master Jan 31, 2019
@egernst egernst mentioned this pull request Feb 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants