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

Fix added bugs and inconsistencies #192

Merged
merged 3 commits into from
Jan 20, 2023

Conversation

craciunoiuc
Copy link
Member

First two commits remove a wrongly added binary and then ignore tool binaries from now on.

The last reverts a function from bubbletea that currently breaks when running in kraftkit.

@craciunoiuc craciunoiuc requested a review from nderjung January 20, 2023 08:32
@craciunoiuc craciunoiuc force-pushed the craciunoiuc/fix-added-bugs branch 2 times, most recently from cfdd167 to 427be41 Compare January 20, 2023 09:04
Comment on lines 171 to 174
// This line will disable linting for the whole file, be careful
//nolint:staticcheck
//lint:ignore SA1019 using Sequence is currently breaking inside bubbletea
return md, tea.Sequentially(tea.Batch(cmds...), tea.Quit)
Copy link
Member

Choose a reason for hiding this comment

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

This issue arises from upgrading bubbletea which exposes a new API. Since using this API throws a nil pointer exception and instead of ignoring the underlying issue and using the older now deprecated API by ignoring all issues in the file a separate issue in itself, we should address the nil pointer directly. Taking a look at how the new API from bubbletea works shows that the nil will be returned by tea.Batch if the list of cmds is empty. So we can check for this return type and simply tea.Quit if this is the case:

		batch := tea.Batch(cmds...)
		if batch == nil {
			return md, tea.Quit
		}

		return md, tea.Sequence(batch, tea.Quit)

Please adapt the above explanation text in the git commit message which fixes this issue.

Copy link
Member

Choose a reason for hiding this comment

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

This fixes a crash for all commands caused by bfaa6e0.
The crash happens inside a dependency and is caused by the
the combination of `Batch` and `Sequence` functions.
`Batch` can now return `nil` which needs to be handled.

Signed-off-by: Cezar Craciunoiu <[email protected]>
@craciunoiuc craciunoiuc force-pushed the craciunoiuc/fix-added-bugs branch from 427be41 to 222af7f Compare January 20, 2023 10:58
Copy link
Member

@nderjung nderjung left a comment

Choose a reason for hiding this comment

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

Thanks!

@nderjung nderjung merged commit f283c75 into unikraft:staging Jan 20, 2023
@nderjung nderjung added this to the v0.4.0 milestone Jan 25, 2023
@craciunoiuc craciunoiuc deleted the craciunoiuc/fix-added-bugs branch April 20, 2023 09:02
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.

2 participants