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

Add support for Jinja templates, closes #119 #120

Merged
merged 6 commits into from
Aug 1, 2019

Conversation

pabrahamsson
Copy link
Contributor

What does this PR do?

Adds support for using Jinja templates

How should this be tested?

ansible-playbook playbooks/openshift-cluster-seed.yml -i tests/inventories/jinja-templates

This should create 6 new projects

$ oc get project
NAME                                  DISPLAY NAME                                                               STATUS
...
oa-ci-jinja-templates-file-dev        OpenShift Applier Jinja Templates Test 1 (displayName) (file) - dev        Active
oa-ci-jinja-templates-file-prod       OpenShift Applier Jinja Templates Test 1 (displayName) (file) - prod       Active
oa-ci-jinja-templates-file-test       OpenShift Applier Jinja Templates Test 1 (displayName) (file) - test       Active
oa-ci-jinja-templates-template-dev    OpenShift Applier Jinja Templates Test 1 (displayName) (template )- dev    Active
oa-ci-jinja-templates-template-prod   OpenShift Applier Jinja Templates Test 1 (displayName) (template )- prod   Active
oa-ci-jinja-templates-template-test   OpenShift Applier Jinja Templates Test 1 (displayName) (template )- test   Active
...

Is there a relevant Issue open for this?

#119

Who would you like to review this?

cc: @redhat-cop/openshift-applier

@malacourse
Copy link

Any thoughts on just using a .j2 extension rather than setting the jinja variable?

@pabrahamsson
Copy link
Contributor Author

This sounds fine to me @malacourse but I'd like to hear from @oybed, @etsauer and @makentenza before proceedeing. I know they had some thoughts around this when jinja2 support was first suggested.

@oybed
Copy link
Contributor

oybed commented Jun 18, 2019

@pabrahamsson thank you for this - looks like a good start. I do like the suggestion @malacourse had above about using the .j2 extension to determine if a "pre-processing" is needed. However, I'd like to take it even one step further. The Jinja processing is pretty much the same regardless of file or template, so just make a process_jinja file that handles this - and it only does so if the file or template ends in .j2. In that way, a sample inventory could be:

openshift_cluster_content:
- object: mycontent
  content:
  - template: "{{ inventory_dir }}/../../files/templates/projectrequest.yml"
    params: "..."
    action: create
- object: my-jina-content
  content:
  - template: "{{ inventory_dir }}/../jinja-template.j2"
    params: "..."
  - file: "{{ inventory_dir }}/../jinja-file.j2"

In the inventory above, the file and template steps are handled as-is today, but because the filename ends in .j2 there's a pre-processing step for Jinja.

This process_jinja step could be injected around line 38 in the process-one-entry.yml file.
https://github.com/redhat-cop/openshift-applier/blob/master/roles/openshift-applier/tasks/process-one-entry.yml#L38

Please let me know if anything is unclear.

@oybed
Copy link
Contributor

oybed commented Jun 25, 2019

@pabrahamsson looks good. However, as the CI indicates, it's failing due to not finding the file correctly. Basically, the logic to determine if this is a URL, local file or existing openshift template isn't aware of this new .j2 processing (and hence the "generated file"). This logic needs a bit of refactoring anyway, so I can assist with this when I can find some time.

@oybed oybed self-requested a review July 12, 2019 15:53
@oybed
Copy link
Contributor

oybed commented Jul 31, 2019

@pabrahamsson I finally had a chance to look into the issue and review your PR. So the CI job is correct that this doesn't work as-is for remote hosts. While I still believe we should look to refactor the file location detection, the issue in your PR can be worked around by adding delegate_to: localhost in the process-jinja.yml file. Since you cannot apply delegate_to to the actual include_tasks, you'll have to just create a block in the file and apply it within the file. This will take care of the CI issue. I.e.:

- block:
  - name: Set file or template fact
   :
   : 
  - name: Update path
    set_fact: {"{{ process_file_or_template }}":"{{ dest_path }}"}

  # applies to the entire file
  delegate_to: localhost 

@etsauer
Copy link
Contributor

etsauer commented Aug 1, 2019

We have a winner!!!

Copy link
Contributor

@etsauer etsauer left a comment

Choose a reason for hiding this comment

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

LGTM

@pabrahamsson pabrahamsson added the enhancement New feature or request label Aug 1, 2019
Copy link
Contributor

@oybed oybed left a comment

Choose a reason for hiding this comment

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

LGTM - merging

@oybed oybed merged commit 1b87b99 into redhat-cop:master Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants