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

[WIP] Decouple display from config initialization #43519

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

abadger
Copy link
Contributor

@abadger abadger commented Aug 1, 2018

This is an attempt to give ansible the ability to print out errors with
the config code. It does succeed in decoupling display from config.
Unfortunately, the structure of config and cli currently means that you
have to load a bunch of other pieces of code which need config before
you get to process the code which could print out the config errors.
Since those fail without config, those cause the whole thing to break
down.

It could be worthwhile to continue in this vein, reorganizing the code
so that we can do all of our static loading (option parsing and config
loading) up front before moving on to the rest of the code which needs
to utilize config but before that happens I hope to move display() to
using a logging framework. If that happens, we won't need this anyway.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

utils/display.py

ANSIBLE VERSION
devel 2.6 2.5
ADDITIONAL INFORMATION

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.7 This issue/PR affects Ansible v2.7 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Aug 1, 2018
@ansibot
Copy link
Contributor

ansibot commented Aug 1, 2018

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/utils/display.py:93:9: E126 continuation line over-indented for hanging indent
lib/ansible/utils/display.py:105:9: E123 closing bracket does not match indentation of opening bracket's line

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Aug 1, 2018
This is an attempt to give ansible the ability to print out errors with
the config code.  It does succeed in decoupling display from config.
Unfortunately, the structure of config and cli currently means that you
have to load a bunch of other pieces of code which need config before
you get to process the code which could print out the config errors.
Since those fail without config, those cause the whole thing to break
down.

It could be worthwhile to continue in this vein, reorganizing the code
so that we can do all of our static loading (option parsing and config
loading) up front before moving on to the rest of the code which needs
to utilize config but before that happens I hope to move display() to
using a logging framework.  If that happens, we won't need this anyway.
@abadger abadger force-pushed the decouple-display-from-config-initialization branch from bc8ef4e to cc8e72b Compare August 1, 2018 14:36
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Aug 1, 2018
Copy link
Member

@nitzmahone nitzmahone left a comment

Choose a reason for hiding this comment

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

one comment that's no longer relevant (I think?), otherwise LGTM

""" Display a message to the user

Note: msg *must* be a unicode string to prevent UnicodeError tracebacks.
Note: color is deprecated. Use type instead.
Copy link
Member

Choose a reason for hiding this comment

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

no longer relevant?

@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Aug 2, 2018
@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 Aug 10, 2018
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Nov 25, 2018
@ansibot ansibot added pre_azp This PR was last tested before migration to Azure Pipelines. 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 Dec 6, 2020
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Oct 24, 2023
@ansibot ansibot added the stale_pr This PR has not been pushed to for more than one year. label Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.7 This issue/PR affects Ansible v2.7 bug This issue/PR relates to a bug. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html pre_azp This PR was last tested before migration to Azure Pipelines. stale_pr This PR has not been pushed to for more than one year. support:core This issue/PR relates to code supported by the Ansible Engineering Team. WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants