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

Return correct values when running yarn in check mode #153

Merged
merged 3 commits into from
Jun 17, 2020
Merged

Return correct values when running yarn in check mode #153

merged 3 commits into from
Jun 17, 2020

Conversation

imjoseangel
Copy link
Contributor

SUMMARY

Fixes #152

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

yarn module

ADDITIONAL INFORMATION

When running check mode on yarn module, it returns an empty string by default when it expects two values for err and out. For instance:

out, err = yarn.install()

Currently the return is like:

return ''

and should be at least:

return(None, None)

From my point of view, this module needs to be have some checks for idempotency and dry-runs, but for now this will fix the issue. If you think it is interesting refactoring it, let me know and I can work on it.

After the change

changed: [localhost] => {
    "changed": true,
    "err": null,
    "invocation": {
        "module_args": {
            "executable": null,
            "global": true,
            "ignore_scripts": false,
            "name": "now",
            "path": null,
            "production": false,
            "registry": null,
            "state": "present",
            "version": null
        }
    },
    "out": null
}

@imjoseangel imjoseangel changed the title Bug/checkyarnreturn Bug/check_yarn_return_oncheck Apr 11, 2020
@imjoseangel imjoseangel changed the title Bug/check_yarn_return_oncheck bug/check_yarn_return_oncheck Apr 11, 2020
@Akasurde Akasurde changed the title bug/check_yarn_return_oncheck Return correct values when running yarn in check mode Apr 12, 2020
@Akasurde
Copy link
Member

@imjoseangel Thanks for the PR. Could you please add a change log entry and integration test for this change? Thanks.

@imjoseangel
Copy link
Contributor Author

@Akasurde added simple tests for check mode when installing all packages. I think this module can be refactored with indempotency and BDD in mind in a different PR but I don't know if the effort worths it.

What do you think?

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added affects_2.10 bug This issue/PR relates to a bug has_issue integration tests/integration module module new_contributor Help guide this first time contributor tests tests stale_ci CI is older than 7 days, rerun before merging labels Apr 17, 2020
@ansibullbot ansibullbot removed the new_contributor Help guide this first time contributor label Jun 16, 2020
@felixfontein felixfontein merged commit 5401452 into ansible-collections:master Jun 17, 2020
@felixfontein
Copy link
Collaborator

@imjoseangel thanks for fixing this!

@gundalow gundalow added the pr_day Has been reviewed during a PR review Day. https://github.com/ansible/community/issues/407 label Jun 17, 2020
@imjoseangel imjoseangel deleted the bug/checkyarnreturn branch July 30, 2020 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug community_review has_issue integration tests/integration module module pr_day Has been reviewed during a PR review Day. https://github.com/ansible/community/issues/407 stale_ci CI is older than 7 days, rerun before merging tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

yarn module throws an error when running with --check flag
5 participants