Skip to content

Log error dependency resolver exceptions strings #81607

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

Open
wants to merge 7 commits into
base: devel
Choose a base branch
from

Conversation

webknjaz
Copy link
Member

SUMMARY

$sbj.

ISSUE TYPE
  • Maintenance Pull Request
ADDITIONAL INFORMATION

N/A

@webknjaz webknjaz requested review from jborean93 and s-hertel August 31, 2023 00:49
@ansibot ansibot added the needs_triage Needs a first human triage before being processed. label Aug 31, 2023
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Aug 31, 2023
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Sep 14, 2023
@jborean93
Copy link
Contributor

@webknjaz are you still interested in this PR?

@ansibot ansibot added the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. label Dec 20, 2024
@webknjaz

This comment was marked as outdated.

This comment was marked as outdated.

@ansibot ansibot removed the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. label Jan 21, 2025
@webknjaz

This comment was marked as resolved.

@webknjaz webknjaz added the unimportant_ci This PR does not need to have healthy CI status and should be ignored by the CI infra maintainers. label Jan 21, 2025
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jan 21, 2025
@ansibot ansibot added the stale_pr This PR has not been pushed to for more than one year. label Jan 28, 2025
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 4, 2025
@webknjaz webknjaz force-pushed the ux/log-depresolver-exception-str branch from aac9f8e to 59884c9 Compare February 5, 2025 02:18
@ansibot ansibot removed stale_pr This PR has not been pushed to for more than one year. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Feb 5, 2025
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 19, 2025
@webknjaz webknjaz force-pushed the ux/log-depresolver-exception-str branch from 59884c9 to 63fe6d0 Compare February 20, 2025 22:54
@webknjaz webknjaz requested a review from bcoca February 20, 2025 22:55
@webknjaz
Copy link
Member Author

@jborean93 @s-hertel can I get approvals plz?

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Feb 20, 2025
@webknjaz webknjaz force-pushed the ux/log-depresolver-exception-str branch from 63fe6d0 to 7dc422f Compare February 27, 2025 23:03
@webknjaz webknjaz removed the unimportant_ci This PR does not need to have healthy CI status and should be ignored by the CI infra maintainers. label Feb 27, 2025
@webknjaz webknjaz moved this from 🫸In review🫷 to ⏰ Review reminder needed 👀 in 📅 Procrastinating in public Feb 27, 2025
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Feb 27, 2025
@webknjaz webknjaz moved this from ⏰ Review reminder needed 👀 to 🫸In review🫷 in 📅 Procrastinating in public Mar 3, 2025
@jborean93
Copy link
Contributor

@webknjaz do you have an example of the type of error messages this would produce. Is it worth using display.error(...) here instead of display.display(...)?

Also are you able to add a changelog fragment for this new change?

@webknjaz
Copy link
Member Author

webknjaz commented Mar 5, 2025

@jborean93 So @bcoca asked me to switch away from error @ 86cb2f5 (#81607). This is because raising Ansible Error later on will already produce error output.

@webknjaz
Copy link
Member Author

webknjaz commented Mar 5, 2025

do you have an example of the type of error messages this would produce

I'll need to see how to trigger it. It was long ago when I looked into this. I just remember that when I was hitting these code branches, it didn't seem like there was enough information in the output to understand what was going on.

@mattclay
Copy link
Member

mattclay commented Mar 5, 2025

Using display.error is probably the right choice here, especially with the improved error handling in DT.

@webknjaz
Copy link
Member Author

webknjaz commented Mar 5, 2025

Using display.error is probably the right choice here, especially with the improved error handling in DT.

@mattclay okay, I can revert that commit.


@jborean93 I was able to trigger one of the code paths and checked the output. I realized it's rather raw:

$ ansible-galaxy collection install 'vmware.vmware:<1.5' 'vmware.vmware:>1.5' -p .                                                                                                             
[WARNING]: You are running the development version of Ansible. You should only run Ansible from "devel" if you are modifying the Ansible engine, or trying out features under development. This is a
rapidly changing source of code and can become unstable at any point.
Starting galaxy collection install process
[WARNING]: The specified collections path '/home/wk/src/github/ansible/ansible' is not part of the configured Ansible collections paths
'/home/wk/.ansible/collections:/usr/share/ansible/collections'. The installed collection will not be picked up in an Ansible run, unless within a playbook-adjacent collections directory.
Process install dependency map
Collection dependency resolution impossible: [RequirementInformation(requirement=<vmware.vmware:<1.5 of type 'galaxy' from Galaxy>, parent=None), RequirementInformation(requirement=<vmware.vmware:>1.5 of type 'galaxy' from Galaxy>, parent=None)]
ERROR! Failed to resolve the requested dependencies map. Could not satisfy the following requirements:
* vmware.vmware:<1.5 (direct request)
* vmware.vmware:>1.5 (direct request)
Hint: Pre-releases hosted on Galaxy or Automation Hub are not installed by default unless a specific version is requested. To enable pre-releases globally, use --pre.

As for the InconsistentCandidate case, I'm not sure how to trigger it without monkey-patching the provider. Looking into resolvelib suggests that it's happening when find_matches() returns incompatible candidates.

I discovered that some resolvelib exceptions define __str__() that render repr()s of things. So I guess it does make sense to log them in debug mode. Or perhaps use display.vvvvv(). Plus, it seems like the existing user-facing output is good enough.

This PR is so old that I didn't even remember what I was after. But going through it once again, I think it was difficult to understand what exceptions in the error handling paths happen. And logging those details is still useful but I'd do that with increased verbosity only.

I also realized that there's a number of other exceptions in resolevlib that we don't explicitly handle but probably should. So I think I'll look into making more changes to this PR.

@webknjaz webknjaz moved this from 🫸In review🫷 to 🚧 In progress 🚧 in 📅 Procrastinating in public Mar 5, 2025
@jborean93
Copy link
Contributor

Makes sense, I would be weary of adding more output to the command as well. The sheer number of verbosity levels probably hasn't been the best things and adding more noise may not be that great. Certainly more debug information for when things go wrong would be nice but we don't want to overload the user with too much information.

@webknjaz
Copy link
Member Author

webknjaz commented Mar 6, 2025

@jborean93 I'll go for vvvvv() then.

@webknjaz webknjaz force-pushed the ux/log-depresolver-exception-str branch from 7dc422f to fb22a24 Compare March 11, 2025 14:37
@webknjaz
Copy link
Member Author

@jborean93 so I experimented with surfacing better information regarding errors here and there. Plz take a look.

@webknjaz webknjaz moved this from 🚧 In progress 🚧 to 🫸In review🫷 in 📅 Procrastinating in public Mar 17, 2025
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 25, 2025
@webknjaz webknjaz moved this from 🫸In review🫷 to ⏰ Review reminder needed 👀 in 📅 Procrastinating in public Apr 1, 2025
@webknjaz
Copy link
Member Author

webknjaz commented Apr 1, 2025

@jborean93 @s-hertel could you find a minute to share more thoughts on this?

@webknjaz
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 12, 2025
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.17 stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants