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 panic when calling parents #72

Merged
merged 2 commits into from
Jan 28, 2022
Merged

Fix panic when calling parents #72

merged 2 commits into from
Jan 28, 2022

Conversation

micprog
Copy link
Member

@micprog micprog commented Jan 18, 2022

In case a dependency doesn't have a manifest, the parents command panicks as it cannot read the dependencies from the manifest. This fixes the issue, simply ignoring these dependencies.

Copy link
Contributor

@andreaskurth andreaskurth left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

In case a dependency doesn't have a manifest, the parents command
panicks as it cannot read the dependencies from the manifest. This
fixes the issue, simply ignoring these dependencies.
When the Bender.lock and the manifests do not match, bender can panic.
This gives a slightly clearer indication to what the error is.
@micprog
Copy link
Member Author

micprog commented Jan 18, 2022

After further testing in the hero repo I found another bug. These changes should clarify the source of the issue, either a missing manifest or the dependency not listed in the manifest, but the lock file contains it (i.e. mismatched lock file in both cases).

Copy link
Contributor

@andreaskurth andreaskurth left a comment

Choose a reason for hiding this comment

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

Would it not be cleaner to return an early Err from the run function of the parents subcommand, instead of inserting dummy entries as in this implementation?

@micprog
Copy link
Member Author

micprog commented Jan 27, 2022

Would it not be cleaner to return an early Err from the run function of the parents subcommand, instead of inserting dummy entries as in this implementation?

I agree the dummy entries are not the cleanest solution, however I would refrain from explicitly returning errors here as it would reduce the usability of the parents command. The problem mainly arises when the Bender.yml files are out of sync with a repository making use of the checkout_dir functionality. In this case, it is possible that a bender update will add certain dependencies to the lock file that are no longer required by the checked out dependencies, resulting in a missing link. I think the most sensible solution would be to properly update the resolution if checkout_dir is used, ensuring these Bender.yml files are used for resolution instead of the upstream git dependencies. Once this is fixed, the missing links should disappear and no longer result in dummy entries, so these missing links can return errors as they should no longer be called. For now, however, I would like to keep a functionality like this (could be altered to warnings), such that the feature is still usable and does not by default result in an error, removing even the partial functionality.

@andreaskurth
Copy link
Contributor

I understand and agree, thanks!

@micprog
Copy link
Member Author

micprog commented Jan 28, 2022

Added an issue #74 to track this problem, thanks for the review!

@micprog micprog merged commit 0a10deb into master Jan 28, 2022
@micprog micprog deleted the fix_parents_panic branch March 25, 2022 12:44
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.

None yet

2 participants