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

Make sure we detect dependency installation failure #3105

Merged

Conversation

tadeboro
Copy link
Contributor

@tadeboro tadeboro commented May 9, 2021

Right now, we just ignore the failure because we do not check the return code of the children's process. This commit just makes sure we are notified when the dependency command fails.

Fixes #3070

PR Type

  • Bugfix Pull Request

Right now, we just ignore the failure because we do not check the
return code of the children process. This commit just makes sure we
are notified when the dependency command fails.
@tadeboro tadeboro force-pushed the exit-on-failed-dependency-installation branch from 539845b to 3b3a8a7 Compare May 9, 2021 14:01
@ssbarnea ssbarnea added the bug label May 14, 2021
@ssbarnea ssbarnea merged commit 288dade into ansible:master May 14, 2021
@tadeboro tadeboro deleted the exit-on-failed-dependency-installation branch May 14, 2021 11:58
@fourstepper
Copy link
Contributor

In GitLab CI, it is quite easy to pull dependencies from gitlab over HTTPs without a secret by setting the git config like this: git config --global url."https://gitlab-ci-token:${CI_JOB_TOKEN}@gitlab.com/".insteadOf https://gitlab.com/

As I have this set, this PR is a regression for me, as molecule just fails on dependency resolution, when before I could just enter away at it and it would pick up roles downloaded through a different requirements file (over SSH)

Is it possible to change the behaviour to either not make this default or make it optional to check successful dependency download? Perhaps with an environment variable?

@ssbarnea
Copy link
Member

I not think failure to install deps should be ignored but if you propose a PR that sorts your use case without affecting other users I will likely accept it.

@tadeboro
Copy link
Contributor Author

Silently ignoring the fact that dependencies were not installed is not something one should do by default. I would suggest that you parameterize your molecule configuration file using an environment variable that controls whether dependency installation should be attempted or not. Something like this should work:

dependency:
  name: galaxy
  enabled: ${INSTALL_DEPS:-true}

Then we can set the INSTALL_DEPS variable to false if we would like to skip the dependency installation.

@fourstepper
Copy link
Contributor

You are right. By default we shouldn't ignore dependency installation failures. This is a good enough solution. Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for checking exit codes on shell dependencies
3 participants