Update Github Action template

[mathias-luedtke]

The current matrix expansion approach works, but it is not the intended usage and results in a warning.

Matrix options must only contain primitive values

[gavanderhoorn]

This should be equivalent:

jobs:
  industrial_ci:
    strategy:
      matrix:
        ros_distro: [ melodic ]
        ros_repo: [ main, testing ]

    ...

    steps:
      - ...

      - uses: ros-industrial/industrial_ci@master
        env:
          ROS_DISTRO: ${{ matrix.ros_distro }}
          ROS_REPO: ${{ matrix.ros_repo }}

      - ...

I haven't really found a way yet to provide 'defaults' in the matrix section.

Could set env.ROS_REPO to main perhaps and then rely on the matrix expansion to override it when desired?

[mathias-luedtke]

Yes, this is what I had in mind.
I just don't like the redundancy..

We could extend the action to read the matrix variable and then deal with the defaults (from env.*).

[mathias-luedtke]

@gavanderhoorn:

How do you like this option (#591)?

jobs:
  industrial_ci:
    strategy:
      matrix:
        ROS_DISTRO: [ melodic ]
        ROS_REPO: [ main, testing ]

    ...

    steps:
      - ...

      - uses: ros-industrial/industrial_ci@master
        with:
           matrix: ${{ toJSON(matrix) }}        
      - ...

[gavanderhoorn]

Hm.

I actually liked the fact you have to explicitly tell each step (and in this case the industrial_ci step) where the values come from.

It's really obvious reading the .yml that step gets two env vars: ROS_DISTRO and ROS_REPO, and their values come from the matrix stanza earlier in the file.

With your proposal, you have to understand how toJSON(..) works and what gets passed exactly.

That's of course OK, but if we can get something equivalent (and I believe the default way to do this is) and it's easier to grok, it may be preferable over saving a line (or two, or three ..).

Do we expect the matrix to contain lots of variables / options? If yes: then your proposal would avoid significant duplication. If average configurations will only have a few rows in the matrix, and the rest of the env vars specified either globally or on the industrial_ci step itself, then perhaps having things explicitly mapped (ie: as in #590 (comment)) might not be so bad.

[mathias-luedtke]

I actually liked the fact you have to explicitly tell each step (and in this case the industrial_ci step) where the values come from.

You can still do this:

jobs:
  industrial_ci:
    strategy:
      matrix:
        include:
          - { ROS_DISTRO: melodic }
          - { ROS_DISTRO: melodic, ROS_REPO: main }

    ...

    steps:
      - ...

      - uses: ros-industrial/industrial_ci@master
        with
          matrix: ${{ toJSON(matrix) }}        
      - ...

Or even a mix of both. And the env hack still works.

It's really obvious reading the .yml that step gets two env vars: ROS_DISTRO and ROS_REPO, and their values come from the matrix stanza earlier in the file.

The env hack in the current template is a not supported/intended use case (by Github Actions).
They might reject this configuration in the future. (Or support it..).

My intention was to support env overiding as well:

      - uses: ros-industrial/industrial_ci@master
        with
          matrix: ${{ toJSON(matrix) }}
        env:
           CCACHE_DIR: ...

With your proposal, you have to understand how toJSON(..) works and what gets passed exactly.

It would be great to avoid toJSON, but the action fails somewhere in the middle because of some validation check.
Again, this is optional, just for actual matrix operations.

[tylerjw]

I'm wondering how I would create this sort of configuration with the examples above:

          - {ROS_DISTRO: foxy, ROS_REPO: main, CLANG_TIDY: pedantic}
          - {ROS_DISTRO: foxy, ROS_REPO: testing}
          - {ROS_DISTRO: rolling, ROS_REPO: main}

I only want to run clang-tidy on one of them because it takes a long time to run.

[mathias-luedtke]

I'm wondering how I would create this sort of configuration

This is not possible with a matrix, but you can include them individually.

[gavanderhoorn]

@tylerjw: I believe that may actually be possible using include. See Example including new combinations in the GHA docs.

[destogl]

@ipa-mdl, @gavanderhoorn do you have a "recommended" approach in the future? I would then proceed with it. I like to keep all my configurations in line to reduce thinking when looking at them after a year or so 😄

If no and you accept votes I am for the one from the second comment.

[gavanderhoorn]

It depends a bit on the size of the configuration I believe.

Explicitly passing all entries as env vars to your industrial_ci step is nice (and clearly shows what's going on), but I'm not sure it scales.

The configurations I've used and created so far were not too extensive, so what I showed in #590 (comment) still worked.

I could imagine larger nrs of env vars getting unwieldy pretty quickly though, so passing the complete dict could be more efficient then.

[mathias-luedtke]

do you have a "recommended" approach in the future?

@destogl: That's exactly what we want to figure out in this issue ;)

For industrial_c itself I will keep the toJSON approach, because we try to cover all possible options.

IMHO for simpler cases (normal repositories) @gavanderhoorn's solution is better, because it is states exactly what's happening.