anaconda icon indicating copy to clipboard operation
anaconda copied to clipboard

Branching helper

Open OndrejZobal opened this issue 2 years ago • 4 comments

A new script to help for generating infrastructure configs with values from a unified template.

Usage

  • The script is found at scripts/branching-helper and the variable file at scripts/template_variables.yml.
  • The scripts currently recursively traverses directories up from cwd and looks for files who's names match /^.?infra-templ-/ and renders it into a file in the same path with the pattern removed.
  • The templater Jinja2 is configured to replace variables [$ in_these_brackets $] and logic/controll structures [% in these %] otherwise we use normal Jinja syntax.

In other words if you would like to create a new template for an existing file and render it you would:

  • mv file infra-templ-file
  • vim infra-template-file # replacing constants with jinja variables
  • scripts/branching-helper And now the contents of file should reflect the rendered template.

All details about how this script works are a subject to discussion and change.

The original version of the script that downloaded variables over http and stored template configs in a separate file tree can be found at this commit: d576b5e4677b4f69eaefdbedd2b7a56a8b371aa8


Possible alternative (without separate template files)

I get why using templates isn't ideal so i thought about some more approaches worth exploring. One would be to use special comments either above or on lines that require changing between branches something like:

# @replace git_branch
branch: master

The script would have to understand the syntax somewhat which could probably just be some regex and the program could just which set of regexes to use based on the file name extension Formats like yaml however contain more complex syntax such as lists and that would complicate things further.

So a safer iteration might be using a dumbed down search and replace again triggered by a comment on the line above:

# @replace master git_branch
branch: master

This would also require updating the comment in order to update the previously inserted value.

Or we could keep using the templating library and combine it with the comments.

# @replace branch: [$ git_branch $]
branch: master

I think it's worth discussing a little more

OndrejZobal avatar Aug 11 '22 10:08 OndrejZobal

The third option looks really interesting to me. However, I wonder how we can solve multilines changes. Maybe adding the whole jinja blocks could be nice improvement to that?

It would be helpful for example here: https://github.com/rhinstaller/anaconda/blob/master/.github/workflows/container-autoupdate.yml#L28

Example of my idea:

#@@@ template-block @@@
# [$ for item in navigation $]
# {{ branch_name }}:
#    if: false
#    uses: ./.github/workflows/container-rebuild-action.yml
#    secrets: inherit
#      with:
#        container-tag: {{ branch_name }}
#        branch: {{ branch_name }}
# [$ endfor $]
#@@@ generated @@@
  f36-devel:
    if: false
    uses: ./.github/workflows/container-rebuild-action.yml
    secrets: inherit
    with:
      container-tag: f36-devel
      branch: f36-devel
#@@@ template-block-end @@@

I wonder how hard this would be for implementation.

jkonecny12 avatar Aug 11 '22 11:08 jkonecny12

I would also mv:

scripts/template_variables.yml --> branch-variables.yml

could be useful elsewhere.

jkonecny12 avatar Aug 11 '22 11:08 jkonecny12

Maybe adding the whole jinja blocks could be nice improvement to that?

I think this blocks are a great addition to the one-liner and we wouldn't loose any functionality over the current solution with the templates. The implementation will be trick but It's definitely doable.

YAML in particular is very picky about spaces though, so that could also be tricky to get right. A good solution to this with the one-liners would be just copying the white characters before the pound sign onto the generated line and the blocks cloud essentially do something similar.

But I think it is the best solution.

We also need to make that the special tokens we chose won't conflict with any format we will use it with (or aren't hard to type).


I would also mv: scripts/template_variables.yml --> branch-variables.yml

Sounds much better thanks but also I am not sure if scripts/ is a good place for this file in general.

OndrejZobal avatar Aug 11 '22 12:08 OndrejZobal

  • Great that it's jinja 2, we already use it in the ansible stuff in builders, so at least it's a reusable skill :)
  • I agree the config should be at top level.
  • The file layout should be such that <path-to-file>.j2 generates <path-to-file>. That is, a file's template is right next to the file, same name. That way, it is immediately clear that there is a template, and one does not have to jump around to find the template.
  • For the example generating container rebuilds, that's not right, you want to generate the number 36/37/38 and the if: false/true part, the rest is constant. Keep it simple, no need to generate stuff with loops if you can just replace bits of the existing file here and there.
  • Do we actually need 300 lines of a script to do this? Running jinja for data+template=result on command line is a solved problem. I expected just a Makefile target along these lines:
    reload-infra:
        for template in **/*.j2 ; do \
          jinja2-cli "$$template" branch-variables.yml > "$${template%.j2}" ; \
        done
    
    That's one package away: dnf install python3-jinja2-cli. Let's not reinvent the wheel please?

VladimirSlavik avatar Aug 12 '22 20:08 VladimirSlavik

I agree with the Makefile and removing the script we have if possible.

jkonecny12 avatar Aug 15 '22 14:08 jkonecny12

So I had a look on jinja2-cli and sadly it does not support changing delimiters (and generally doesn't have many features at all) which we need because GitHub workflows use the same syntax and Jinja will try to replace GitHub related variables.

  File "/home/ozobal/rh/anaconda/.github/workflows/build-boot-iso.yml", line 30, in top-level template code
    route: GET /repos/${{ github.repository }}/collaborators/${{ github.event.sender.login }}/permission
  File "/usr/lib/python3.10/site-packages/jinja2/environment.py", line 474, in getattr
    return getattr(obj, attribute)
jinja2.exceptions.UndefinedError: 'github' is undefined

There is a way of "escaping" these by putting them into a string literal but it isn't very pretty:

route: GET /repos/{{ "${{ github.repository }}" }}/collaborators/{{ "${{ github.event.sender.login }}" }}/permission

or using raw sections:

{% raw %}
route: GET /repos/${{ github.repository }}/collaborators/${{ github.event.sender.login }}/permission
{% endraw %}

Another thing worth noting is that jinja2-cli is a third party program maintained by a person who isn't affiliated with Jinja devs.

I think going with a custom script is might be the best option.

I agree that my branching-helper is absolutely bloated but that is because after our first meeting I needed to change how the script works quickly to have a working example to put into this PR, I can definitely make it shorter.

OndrejZobal avatar Aug 16 '22 07:08 OndrejZobal

No issue for keeping our custom script here. I would like to have an existing variant but I agree with the reasons above.

jkonecny12 avatar Aug 16 '22 09:08 jkonecny12

Update

  • I have decided against using jinja2-cli an instead reworked the script to scripts/jinja-render the script now just renders a single file and saves it.
  • I added the make target suggested by @VladimirSlavik, so in order to rerender all files simply run make reload-infra.
  • I also moved the variables to scriptsbranch-variables.yml as suggested by @jkonecny12
  • As @VladimirSlavik suggested the template files will use the *.j2 extension.

OndrejZobal avatar Aug 16 '22 13:08 OndrejZobal

I addressed @jkonecny12's suggestions in the latest commit. So I am going to rebase it now, but I should note that this new script has not yet been documented in readmes/docs.

OndrejZobal avatar Aug 17 '22 09:08 OndrejZobal

Please squash the commits otherwise I'm happy. For the documentation it has to be added to the branching guide. Should be probably done later when we add also all the configuration files.

jkonecny12 avatar Aug 17 '22 10:08 jkonecny12

/kickstart-test --testtype smoke

jkonecny12 avatar Aug 17 '22 15:08 jkonecny12

Missing dependencies are: python3-pyyaml and python3-jinja2.

OndrejZobal avatar Aug 18 '22 09:08 OndrejZobal

/kickstart-test --testtype smoke

poncovka avatar Aug 18 '22 10:08 poncovka

Side note: We will have to think also about which files should be named in .structure-config, but that's fine in a later PR. We are still to find out what needs to be templated, and how we feel about the relationships between the files.

VladimirSlavik avatar Aug 23 '22 08:08 VladimirSlavik