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

fixes broken "make run" for ansible operators #6110

Merged
merged 3 commits into from
Oct 25, 2022

Conversation

mhrivnak
Copy link
Member

Description of the change:

Previously, an ansible operator would be scaffolded with a Makefile whose "run" target had incorrect syntax. It would run this command:

ANSIBLE_ROLES_PATH="$(ANSIBLE_ROLES_PATH):$(shell pwd)/roles" $(ANSIBLE_OPERATOR) run

That command failed to set the ANSIBLE_ROLES_PATH variable correctly because it was using Makefile syntax in a target's recipe, which gets interpreted as shell script.

This commit moves the variable setting statement, which uses Makefile syntax, out of the recipe. It also uses the standard ?= operator to only set the variable if it is unset.

Signed-off-by: Michael Hrivnak mhrivnak@redhat.com

Motivation for the change:

fix the bug

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@mhrivnak mhrivnak temporarily deployed to deploy October 24, 2022 17:27 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 24, 2022 17:27 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 24, 2022 17:27 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 24, 2022 17:27 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 24, 2022 17:27 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 24, 2022 17:27 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 24, 2022 17:27 Inactive
Previously, an ansible operator would be scaffolded with a Makefile
whose "run" target had incorrect syntax. It would run this command:

`ANSIBLE_ROLES_PATH="$(ANSIBLE_ROLES_PATH):$(shell pwd)/roles" $(ANSIBLE_OPERATOR) run`

That command failed to set the `ANSIBLE_ROLES_PATH` variable correctly
because it was using Makefile syntax in a target's recipe, which gets
interpreted as shell script.

This commit moves the variable setting statement, which uses Makefile
syntax, out of the recipe. It also uses the standard `?=` operator to
only set the variable if it is unset.

Signed-off-by: Michael Hrivnak <mhrivnak@hrivnak.org>
@mhrivnak mhrivnak force-pushed the fix-ansible-makefile-run branch from eeead51 to 30512ce Compare October 24, 2022 17:35
@mhrivnak mhrivnak temporarily deployed to deploy October 24, 2022 17:36 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 24, 2022 17:36 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 24, 2022 17:36 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 24, 2022 17:36 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 24, 2022 17:36 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 24, 2022 17:36 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 24, 2022 17:37 Inactive
Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2022
@mhrivnak
Copy link
Member Author

I see that the sanity/sanity test failed, but it's not giving me much info from which to figure out what it was testing or why it failed. If anyone could point me in the right direction, that would be appreciated.

jmrodri and others added 2 commits October 24, 2022 20:01
Signed-off-by: jesus m. rodriguez <jesusr@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2022
@mhrivnak mhrivnak temporarily deployed to deploy October 25, 2022 01:58 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 25, 2022 01:58 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 25, 2022 01:58 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 25, 2022 01:58 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 25, 2022 01:58 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 25, 2022 01:58 Inactive
@mhrivnak mhrivnak temporarily deployed to deploy October 25, 2022 01:58 Inactive
@jmrodri
Copy link
Member

jmrodri commented Oct 25, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2022
@jmrodri jmrodri merged commit b59def5 into operator-framework:master Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants