redhat-cop / openshift-applier

Used to apply OpenShift objects to an OpenShift Cluster
Apache License 2.0
102 stars 61 forks source link

Fix to allow late variable evaluation #138

Closed jkupferer closed 4 years ago

jkupferer commented 4 years ago

What does this PR do?

Allows variables set by set_fact called in a previous content items to be referenced in later content items.

How should this be tested?

Standard applier tests... plus tests not yet developed

Is there a relevant Issue open for this?

resolves https://github.com/redhat-cop/openshift-applier/issues/137

Who would you like to review this?

cc: @redhat-cop/openshift-applier

oybed commented 4 years ago

I'd like to get PR #124 in before this one as we are planning to cut a new v2.1.0 release once that is done. After both of those things have happened, we can review and merge this PR for a post v2.1.0 release.

etsauer commented 4 years ago

FYI, This works well for my use case, and all tests are passing. Let's get on that other PR!

etsauer commented 4 years ago

@jkupferer can you rebase?

jkupferer commented 4 years ago

Updated this PR to resolve conflicts and also found an issue with the previous version we missed last time. Ready for review and retest.

etsauer commented 4 years ago

@jkupferer CI is now failing with:

    TASK [/home/travis/build/redhat-cop/openshift-applier/roles/openshift-applier : Look for unsupported top-level dictionary entries] ***
    An exception occurred during task execution. To see the full traceback, use -vvv. The error was: NameError: global name 'filter_applier_items' is not defined
    fatal: [centos]: FAILED! => {"msg": "Unexpected failure during module execution.", "stdout": ""}
jkupferer commented 4 years ago

@etsauer Ah, I see, missed a spot renaming filter_applier_items, found and fixed it.

oybed commented 4 years ago

@jkupferer FYI added the keyword resolves to the original comment to auto-resolve the issue when this PR gets merged

oybed commented 4 years ago

@jkupferer @etsauer with the change to the filter plugin, we are now making the openshift-applier do more, and hence be less efficient. The intent for the filter in the first place was to filter out and slim down the inventory up front and hence avoid doing processing on items that aren't even valid for the run. This seems unrelated to what this PR is for, however, so I am not sure I fully agree with the change as-is. I do wonder, however, that there may potentially be a different way to solve this without touching the filter.

etsauer commented 4 years ago

@oybed @jkupferer actually... looking at the code again, I don't see why we couldn't just do something like:

- set_fact:
    openshift_cluster_content: "{{ openshift_cluster_content | filter_applier_items(include_tags, exclude_tags) | default([]) }}"

And then just use @jkupferer 's looping method on that. Then I think we can just throw out the change to the filter and to process-content.yml

jkupferer commented 4 years ago

@etsauer I don't think that will work. As soon as you pass the openshift_cluster_content through a filter it is going to evaluate all the variables and those will be frozen in time. The only way to get late processing that I can think of is to avoid passing this var through a filter.

oybed commented 4 years ago

@etsauer yeah, I'm hoping that it will be something similar to that - and that's what I am hoping to find some time to investigate a bit further.

jkupferer commented 4 years ago

@etsauer @oybed Calling set_fact definitely expands variable references and prevents the sort of late evaluation we are counting on to make gathering facts from the cluster useful.

oybed commented 4 years ago

@jkupferer ok, that's a good piece of info to know. I do still wonder, however, if there may just be a different approach to this. I'll do my best to find some time to dig in a bit further.

oybed commented 4 years ago

After further consideration, I wrote issue #139 to continue this work. Will merge as-is to continue making progress.