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] safer task results #32480

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

[WIP] safer task results #32480

wants to merge 4 commits into from

Conversation

bcoca
Copy link
Member

@bcoca bcoca commented Nov 2, 2017

SUMMARY

Pass scaled down objects with needed data, but w/o methods

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

task result

ANSIBLE VERSION
2.5

@ansibot ansibot added affects_2.5 This issue/PR affects Ansible v2.5 bugfix_pull_request executor/task_result needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. utils/helpers needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 2, 2017
@gundalow gundalow added the ci_verified Changes made in this PR are causing tests to fail. label Nov 2, 2017
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Nov 2, 2017
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Nov 2, 2017
@ansibot
Copy link
Contributor

ansibot commented Nov 2, 2017

The test ansible-test sanity --test pep8 [?] failed with the following error:

lib/ansible/utils/helpers.py:51:5: E731 do not assign a lambda expression, use a def

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Nov 2, 2017

The test ansible-test sanity --test pep8 [?] failed with the following error:

lib/ansible/utils/helpers.py:35:1: E302 expected 2 blank lines, found 1

click here for bot help

@alikins
Copy link
Contributor

alikins commented Nov 2, 2017

What is this trying to fix?

Does TaskResult need to return the same class types?
Seems like task_executor should be able to return data objects instead of the
full classes (more or less a DTO...). Is something later type checking?

Task/Host classes seem like they should be responsible for providing
a copy of just their data. Ideally they would be using it internally (Task has-a TaskModel, etc).



class Chameleon(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

These (Chameleon, data_object_shim) are kind of unusual classes and method. So the comments/commit msgs need to explain why they are needed and how they accomplish that and why normal approaches are not applicable.

@bcoca
Copy link
Member Author

bcoca commented Nov 2, 2017

I'm trying to avoid having callbacks influence play behaviour, currently they can alter the actual play objects in ways that user would not be able to easily audit, those behaviors should be apparent in playbook itself. This also can be a security issue once callbacks are easier to install, it is better to have the locks in the engine than the callbacks themselves.

As for class types, this is due to some checks in callbacks, not sure if we can/should support that though, i'm still trying to figure out the happy medium.

That is what taskresult itself does, but I was looking for 'shorter' approach than having to create 'data only' shims for every class.

@ansibot
Copy link
Contributor

ansibot commented Nov 2, 2017

The test ansible-test sanity --test pep8 [?] failed with the following errors:

lib/ansible/utils/helpers.py:35:1: E302 expected 2 blank lines, found 1
lib/ansible/utils/helpers.py:72:5: E265 block comment should start with '# '

click here for bot help

@gundalow gundalow added the ci_verified Changes made in this PR are causing tests to fail. label Nov 3, 2017
@abadger
Copy link
Contributor

abadger commented Nov 6, 2017

I've been thinking about this and I'm no longer sure that we should do this because it is a backwards incompatible change. In our tree we only use callbacks for display but 3rd parties could be using those objects to make changes or other introspection that needs the original objects. We need to decide if this is something we feel we can break backwards compatibility on immediately or if we should take a different strategy.

@bcoca bcoca changed the title safer task results [WIP] safer task results Nov 8, 2017
@ansibot ansibot added the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Nov 8, 2017
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Nov 16, 2017
@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 Nov 16, 2017
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 2, 2018
@gundalow gundalow added the candidate_to_close Think we can close this, though need to check with Core label Jan 11, 2019
@ansibot ansibot added the new_plugin This PR includes a new plugin. label Mar 3, 2020
@ansibot ansibot added pre_azp This PR was last tested before migration to Azure Pipelines. and removed ci_verified Changes made in this PR are causing tests to fail. 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 stale_review Updates were made after the last review and the last review is more than 7 days old. and removed has_issue labels Jul 12, 2023
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.5 This issue/PR affects Ansible v2.5 bug This issue/PR relates to a bug. candidate_to_close Think we can close this, though need to check with Core executor/task_queue_manager executor/task_result needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_plugin This PR includes a new plugin. pre_azp This PR was last tested before migration to Azure Pipelines. stale_review Updates were made after the last review and the last review is more than 7 days old. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. utils/helpers 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.

None yet

5 participants