docs icon indicating copy to clipboard operation
docs copied to clipboard

What is a valid matrix value?

Open rotu opened this issue 1 year ago • 19 comments

Code of Conduct

What article on docs.github.com is affected?

https://docs.github.com/en/actions/learn-github-actions/contexts#matrix-context https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs

What part(s) of the article would you like to see updated?

The contents of the matrix context is described as string-valued. But it's not clear if this is always correct. It's also not clear what is legal to provide for a matrix in the first place.

Additional information

e.g. In the example here https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#using-a-matrix-strategy , version is syntactically a number. It's not clear whether one should expect ${{ matrix.version }} to be coerced to a string.

e.g. If I provide an object value to matrix, it's not clear if it should work. The below code seems to set ${{ matrix.foo }} to the object {"key": true}, but the GitHub's workflow editor complains "Matrix options must only contain primitive values":

jobs:
  example_matrix:
    strategy:
      matrix:
        - foo:
          - key: true

rotu avatar Jul 02 '23 05:07 rotu

Thanks for opening this issue. A GitHub docs team member should be by to give feedback soon. In the meantime, please check out the contributing guidelines.

welcome[bot] avatar Jul 02 '23 05:07 welcome[bot]

@rotu Thanks so much for opening an issue! I'll triage this for the team to take a look :eyes:

Also, welcome to the community! 🎉

While this is awaiting review, check our help wanted if you are looking for other ways to contribute.

cmwilson21 avatar Jul 03 '23 14:07 cmwilson21

While this is awaiting review, check our help wanted if you are looking for other ways to contribute.

@cmwilson21 Thanks for the recommendations! Could you please try to get the right eyes on this: https://github.com/community/community/commit/861a04a2da53a507200f1bd2f1ff2ca2be6d9e14#commitcomment-120298975

I tried to contribute what seems to be a simple fix but it looks like this issue is getting lost in the ether and other users have hit the same frustration! The community repo doesn't have an issues page so I'm not sure what the right escalation route is.

https://github.com/community/community/pull/59788 https://github.com/community/community/pull/58479 https://github.com/community/community/pull/58406 https://github.com/community/community/pull/58182

rotu avatar Jul 03 '23 17:07 rotu

I'll triage this for the team to take a look 👀

@cmwilson21 Has this been triaged? What's the status?

rotu avatar Jul 10 '23 14:07 rotu

👋 @rotu - It has been triaged and is on the board for review. We had several out for the US holiday last week. A writer will have eyes on it soon ✨

As for your other question about the Communities repo, I'll ask around 👍

cmwilson21 avatar Jul 11 '23 21:07 cmwilson21

@cmwilson21 Original issue was not resolved. This should be a quick documentation fix. Could you PLEASE escalate?

rotu avatar Sep 18 '23 16:09 rotu

@rotu Sorry about that! Looks like stalebot was acting up. Reopening this issue 👍

cmwilson21 avatar Sep 19 '23 19:09 cmwilson21

The matrix syntax definitely allows for some pretty handy undocumented trickery, which works just fine even though the syntax checker complains about it.

For example, the matrix we arrived at following that discussion — to build a cross-platform C/C++ project without special-casing the CMake-configure and compile steps for each OS — took this form:

jobs:
  build:
    runs-on: '${{ matrix.os.host }}-latest'
    strategy:
      matrix:
        os:
          - { host: windows, shell: msys2 }
          - { host: ubuntu,  shell: bash  }
          - { host: macos,   shell: bash  }
        compiler:
          - { cc: gcc,   cxx: g++ }
          - { cc: clang, cxx: clang++ }
    defaults:
      run:
        shell: '${{ matrix.os.shell }} {0}'
    env:
      CC: ${{ matrix.compiler.cc }}
      CXX: ${{ matrix.compiler.cxx }}

It almost looks like it shouldn't work, but it does! So much for, "must contain only primitive values". (It does make it significantly more difficult to use the include: / exclude: features of the strategy:, though.)

ferdnyc avatar Oct 03 '23 02:10 ferdnyc

Thanks for raising this issue. I'm glad to see that the issue_labeler.yml file was fixed since you commented on it.

If we're going to update this section of the docs to specify supported matrix values, I think we should get input from an SME to ensure that we don't introduce any errors. I'm adding the SME flag to request an SME review.

felicitymay avatar Oct 10 '23 14:10 felicitymay

Thanks for opening an issue! We've triaged this issue for technical review by a subject matter expert :eyes:

github-actions[bot] avatar Oct 10 '23 14:10 github-actions[bot]

For any experts who may end up reviewing this issue:

Ever since we developed that matrix syntax "hack" (and many, many more like it that followed), I've been the guy walking around spoiling everyone's good time by arguing against becoming too reliant on it continuing to work.

"It's undocumented behavior that GitHub clearly didn't intend to support. The documentation makes absolutely no mention of it, there's not a single example of its kind anywhere in GitHub's provided resources, their syntax checker complains about it, and it's clearly an accident that it even works.1

"We shouldn't treat it like a supported feature, because at any moment they could decide to 'fix' the parser, and make it no longer accept matrix: configurations that were never intended to be valid."

Those are (still) my arguments, but it DOESN'T mean I want them to be correct!

I'd be thrilled if this (widely-used and clearly desirable) syntax was finally made official. Perhaps even alternate forms of the same configuration (see below) could also be supported. That'd be an ideal outcome here.

An undesirable outcome would be for Github to prove me right, and finally "fix" the parser to no longer accept this hack. (Breaking untold thousands of users' workflows in the process.)

Notes

Ignore wrong information
  1. I base that part of the argument on the fact that, while this syntax works (apparently as an accident of the parser implementation):

    matrix:
      key1:
        - { subkey1.1: value A, subkey1.2: value M }
        - { subkey1.1: value B, subkey1.2: value N }
      key2:
        - { subkey2.1: value C, subkey2.2: value O }
        - { subkey2.1: value D, subkey2.2: value P }
    

    ... neither of these equivalent syntaxes work:

    matrix:
      key1:
        - subkey1.1: value A
          subkey1.2: value M
        - subkey1.1: value B
          subkey1.2: value N
      key2:
        - subkey2.1: value C
          subkey2.2: value O
        - subkey2.1: value D
          subkey2.2: value P
    
    matrix:
      key1:
        - 
          subkey1.1: value A
          subkey1.2: value M
        - 
          subkey1.1: value B
          subkey1.2: value N
      key2:
        - 
          subkey2.1: value C
          subkey2.2: value O
        - 
          subkey2.1: value D
          subkey2.2: value P
    

    Three different YAML parsers I've tested all produce identical JSON from each of the three forms:

    {
      "matrix": {
        "key1": [
          {
            "subkey1.1": "value A",
            "subkey1.2": "value M"
          },
          {
            "subkey1.1": "value B",
            "subkey1.2": "value N"
          }
        ],
        "key2": [
          {
            "subkey2.1": "value C",
            "subkey2.2": "value O"
          },
          {
            "subkey2.1": "value D",
            "subkey2.2": "value P"
          }
        ]
      }
    }
    

    No matter which YAML form I use, it parses to that exact , identical JSON. Yet, while the first works as a GitHub Actions matrix, the latter two will cause Actions to abort with a workflow syntax error.

ferdnyc avatar Oct 10 '23 19:10 ferdnyc

Ever since we developed that matrix syntax "hack" (and many, many more like it that followed), I've been the guy walking around spoiling everyone's good time by arguing against becoming too reliant on it continuing to work.

Thanks for providing a concrete example! That's exactly why I filed this issue - I saw it a workflow, saw that the syntax checker flagged it, and I didn't know whether it's a feature or a hack.

Related: https://github.com/actions/runner/issues/1660

It's come up in discussions a couple of times: https://github.com/orgs/community/discussions/24981 https://github.com/orgs/community/discussions/27143

rotu avatar Oct 10 '23 21:10 rotu

@rotu Heh. Well, that second discussion is me. :grin:

The first one is interesting, though. I didn't realize that this form also (reportedly) works:

matrix:
  key1: [
    { subkey1.1: value A, subkey1.2: value M },
    { subkey1.1: value B, subkey1.2: value N }
    ]
  key2: [
    { subkey2.1: value C, subkey2.2: value O },
    { subkey2.1: value D, subkey2.2: value P }
    ]

Running that through js-yaml... yup. Also identical:

{
  "matrix": {
    "key1": [
      {
        "subkey1.1": "value A",
        "subkey1.2": "value M"
      },
      {
        "subkey1.1": "value B",
        "subkey1.2": "value N"
      }
    ],
    "key2": [
      {
        "subkey2.1": "value C",
        "subkey2.2": "value O"
      },
      {
        "subkey2.1": "value D",
        "subkey2.2": "value P"
      }
    ]
  }
}

ferdnyc avatar Oct 12 '23 00:10 ferdnyc

@ferdnyc I can confirm the parser intentionally supports complex matrix values and has since 2019. I think it's fairly safe to assume the functionality will not be removed - I imagine it would break a lot of customers' workflows.

When initially added, it was intended to solve scenarios like matching multiple runner labels. I'm happy to see it helped with your scenarios as well (host/shell, etc).

Will help route the issue to address improvements to the docs and validation in the UI editor.

ericsciple avatar Oct 12 '23 20:10 ericsciple

@ferdnyc curiously I was not able to reproduce an error with one of your syntaxes above. This below YAML works for me. Perhaps you had some other syntax error?

on: push
jobs:
  build:
    runs-on: ubuntu-latest
    strategy:
      matrix:
        key1:
          - subkey1.1: Value A
            subkey1.2: Value M
          - subkey1.1: Value B
            subkey1.2: Value N
        key2:
          - subkey2.1: Value C
            subkey2.2: Value O
          - subkey2.1: Value D
            subkey2.2: Value P
    steps:
      - run: echo hi

ericsciple avatar Oct 12 '23 20:10 ericsciple

@ericsciple

@ferdnyc I can confirm the parser intentionally supports complex matrix values and has since 2019. I think it's fairly safe to assume the functionality will not be removed - I imagine it would break a lot of customers' workflows.

Oh, that's good to hear! Yes, I imagine it absolutely would.

@ferdnyc curiously I was not able to reproduce an error with one of your syntaxes above. This below YAML works for me. Perhaps you had some other syntax error?

*sigh* It's certainly possible, it's happened in the past. In re-testing just now, I see that, indeed, all equivalent forms work just fine, and I've been spreading lies again. (Oops.)

Will help route the issue to address improvements to the docs and validation in the UI editor.

That would be great, because the implication is still that this is not how things should be done.

Particularly from the workflow editor, which still red-squiggles both os: and compiler: here, with the complaint that "Matrix options must only contain primitive values":

image

But I've also seen (and of course can't find, now) users trying to work out the correct syntax for their matrix: setup, trying things that are close to the valid syntax, but not quite there, and getting told exactly what the syntax checker says. (Along with, "If you want complex values, use include:". Rather than being pointed to the correct — and apparently, supported! :tada: — syntax for matrix: proper.)

ferdnyc avatar Oct 12 '23 21:10 ferdnyc

Vjvyctuvy

alee599 avatar Oct 23 '23 15:10 alee599

This is a gentle bump for the docs team that this issue is waiting for technical review.

github-actions[bot] avatar Nov 21 '23 16:11 github-actions[bot]

It looks as if the current behavior in workflows is expected to continue to be available.

Updates to the validation of workflows requires internal work. I've created an internal issue to track both the improvements to the validator and the docs. I'm going to put this issue on hold until we're able to give you an update on the internal work.

felicitymay avatar Jan 15 '24 15:01 felicitymay