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

What is a valid matrix value? #26469

Open
1 task done
Tracked by #28822
rotu opened this issue Jul 2, 2023 · 18 comments
Open
1 task done
Tracked by #28822

What is a valid matrix value? #26469

rotu opened this issue Jul 2, 2023 · 18 comments
Labels
actions This issue or pull request should be reviewed by the docs actions team content This issue or pull request belongs to the Docs Content team SME reviewed An SME has reviewed this issue/PR waiting for review Issue/PR is waiting for a writer's review

Comments

@rotu
Copy link

rotu commented Jul 2, 2023

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 rotu added the content This issue or pull request belongs to the Docs Content team label Jul 2, 2023
@welcome
Copy link

welcome bot commented Jul 2, 2023

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.

@github-actions github-actions bot added the triage Do not begin working on this issue until triaged by the team. label Jul 2, 2023
@cmwilson21
Copy link
Contributor

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

Also, welcome to the community! 🎉

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

@cmwilson21 cmwilson21 added actions This issue or pull request should be reviewed by the docs actions team waiting for review Issue/PR is waiting for a writer's review and removed triage Do not begin working on this issue until triaged by the team. labels Jul 3, 2023
@rotu
Copy link
Author

rotu commented Jul 3, 2023

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: community/community@861a04a#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.

community/community#59788
community/community#58479
community/community#58406
community/community#58182

@rotu
Copy link
Author

rotu commented Jul 10, 2023

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

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

@cmwilson21
Copy link
Contributor

👋 @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 👍

@github-actions github-actions bot added the stale There is no recent activity on this issue or pull request label Sep 11, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 18, 2023
@rotu
Copy link
Author

rotu commented Sep 18, 2023

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

@cmwilson21
Copy link
Contributor

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

@cmwilson21 cmwilson21 removed the stale There is no recent activity on this issue or pull request label Sep 19, 2023
@cmwilson21 cmwilson21 reopened this Sep 19, 2023
@github-actions github-actions bot added the triage Do not begin working on this issue until triaged by the team. label Sep 19, 2023
@cmwilson21 cmwilson21 removed the triage Do not begin working on this issue until triaged by the team. label Sep 19, 2023
@ferdnyc
Copy link
Contributor

ferdnyc commented Oct 3, 2023

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.)

@felicitymay
Copy link
Contributor

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 felicitymay added the needs SME This proposal needs review from a subject matter expert. label Oct 10, 2023
@github-actions
Copy link
Contributor

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

@ferdnyc
Copy link
Contributor

ferdnyc commented Oct 10, 2023

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.

@rotu
Copy link
Author

rotu commented Oct 10, 2023

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:
actions/runner#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

@ferdnyc
Copy link
Contributor

ferdnyc commented Oct 12, 2023

@rotu Heh. Well, that second discussion is me. 😁

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"
      }
    ]
  }
}

@ericsciple
Copy link
Member

@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
Copy link
Member

@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

@ferdnyc
Copy link
Contributor

ferdnyc commented Oct 12, 2023

@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! 🎉 — syntax for matrix: proper.)

@alee599

This comment has been minimized.

Copy link
Contributor

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

@github-actions github-actions bot added the SME stale The request for an SME has staled. label Nov 21, 2023
@janiceilene janiceilene added SME reviewed An SME has reviewed this issue/PR and removed SME stale The request for an SME has staled. needs SME This proposal needs review from a subject matter expert. labels Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actions This issue or pull request should be reviewed by the docs actions team content This issue or pull request belongs to the Docs Content team SME reviewed An SME has reviewed this issue/PR waiting for review Issue/PR is waiting for a writer's review
Projects
None yet
Development

No branches or pull requests

8 participants
@rotu @ferdnyc @felicitymay @janiceilene @ericsciple @cmwilson21 @alee599 and others