mpalmer / action-validator

Tool to validate GitHub Action and Workflow YAML files
GNU General Public License v3.0
271 stars 25 forks source link

runs-on custom label is no longer validating #51

Closed deviantintegral closed 1 year ago

deviantintegral commented 1 year ago

This looks to be a regression from https://github.com/mpalmer/action-validator/issues/5 given there's comments there this was fixed.

With the example in the issue:

name: Self-Hosted Ci Job

on:
  workflow_dispatch:
    types:
      - trigger-job
jobs:
  sync:
    runs-on: [custom-runner-name]

I get:

npx @action-validator/cli .github/workflows/self.yml
{
  "actionType": "workflow",
  "errors": [
    {
      "code": "one_of",
      "path": "/jobs/sync",
      "title": "OneOf conditions are not met",
      "states": [
        {
          "errors": [
            {
              "code": "one_of",
              "path": "/jobs/sync/runs-on",
              "title": "OneOf conditions are not met",
              "states": [
                {
                  "errors": [
                    {
                      "code": "wrong_type",
                      "detail": "The value must be string",
                      "path": "/jobs/sync/runs-on",
                      "title": "Type of the value is wrong"
                    },
                    {
                      "code": "enum",
                      "path": "/jobs/sync/runs-on",
                      "title": "Enum conditions are not met"
                    }
                  ]
                },
                {
                  "errors": [
                    {
                      "code": "any_of",
                      "path": "/jobs/sync/runs-on",
                      "title": "AnyOf conditions are not met",
                      "states": [
                        {
                          "errors": [
                            {
                              "code": "const",
                              "path": "/jobs/sync/runs-on/0",
                              "title": "Const condition is not met"
                            }
                          ]
                        },
                        {
                          "errors": [
                            {
                              "code": "const",
                              "path": "/jobs/sync/runs-on/0",
                              "title": "Const condition is not met"
                            },
                            {
                              "code": "min_items",
                              "path": "/jobs/sync/runs-on",
                              "title": "MinItems condition is not met"
                            }
                          ]
                        },
                        {
                          "errors": [
                            {
                              "code": "const",
                              "path": "/jobs/sync/runs-on/0",
                              "title": "Const condition is not met"
                            },
                            {
                              "code": "min_items",
                              "path": "/jobs/sync/runs-on",
                              "title": "MinItems condition is not met"
                            }
                          ]
                        },
                        {
                          "errors": [
                            {
                              "code": "const",
                              "path": "/jobs/sync/runs-on/0",
                              "title": "Const condition is not met"
                            },
                            {
                              "code": "min_items",
                              "path": "/jobs/sync/runs-on",
                              "title": "MinItems condition is not met"
                            }
                          ]
                        },
                        {
                          "errors": [
                            {
                              "code": "const",
                              "path": "/jobs/sync/runs-on/0",
                              "title": "Const condition is not met"
                            },
                            {
                              "code": "min_items",
                              "path": "/jobs/sync/runs-on",
                              "title": "MinItems condition is not met"
                            }
                          ]
                        }
                      ]
                    }
                  ]
                },
                {
                  "errors": [
                    {
                      "code": "wrong_type",
                      "detail": "The value must be object",
                      "path": "/jobs/sync/runs-on",
                      "title": "Type of the value is wrong"
                    }
                  ]
                },
                {
                  "errors": [
                    {
                      "code": "wrong_type",
                      "detail": "The value must be string",
                      "path": "/jobs/sync/runs-on",
                      "title": "Type of the value is wrong"
                    }
                  ]
                }
              ]
            }
          ]
        },
        {
          "errors": [
            {
              "code": "properties",
              "detail": "Additional property 'runs-on' is not allowed",
              "path": "/jobs/sync",
              "title": "Property conditions are not met"
            },
            {
              "code": "required",
              "path": "/jobs/sync/uses",
              "title": "This property is required"
            }
          ]
        }
      ]
    }
  ]
}

Additional property 'runs-on' is not allowed is the error that matters (and the one in my private repository). While the action falis linting, it runs correctly.

mpalmer commented 1 year ago

Howdy!

The issue you link wasn't fixed by any (deliberate) change in action-validator, although the switch to using the JSON schema as a submodule very likely pulled in a newer, fixed schema from upstream.

In any event, I can confirm that I'm getting the same results as you with the example workflow, and I've got a bit of time at the moment to have a poke around and figure out what's going on, so that's my evening spoken for.

mpalmer commented 1 year ago

OK, now I remember what this was all about: the self-hosted label.

The fine manual (now) says:

Although the self-hosted label is not required, we strongly recommend specifying it when using self-hosted runners to ensure that your job does not unintentionally specify any current or future GitHub-hosted runners.

The JSON schema we use encodes that recommendation as a hard requirement, and it appears to be deliberate. I've tested your example workflow with self-hosted added to the beginning of the list, and that validates successfully.

In the short term, I can't see a way to allow action-validator to permit this particular deviation without a lot of work. Forking the upstream schema and relaxing just this requirement is beyond the scope of what I'm willing to take on. If-and-when action-validator grows ignoreable problems, or different levels of problems (info/warn/err/etc), this is the sort of thing that would go well into that sort of framework, but I don't need that (at present, anyway), so it'll be a pr-welcome thing for someone to chew on should they feel suitably strongly about it.

If you have any other ideas about how this could be solved, I'm all ears (eyes?).

deviantintegral commented 1 year ago

We're actually using GitHub's larger hosted runners that are currently in beta. In that case, I would think we wouldn't want to specify self-hosted?

mpalmer commented 1 year ago

Aaah! OK, that's a different problem, I got distracted by remembering the full context around #5.

Not having access to larger runners, I'm not able to do any testing of my own, so I'll need to rely on the docs and what you can tell me about how it actually works to be able to sort this out.

I can't find a list of pre-defined labels that are used to specify "please use a larger runner". Is that correct, or have I not looked in the right spots? It appears that the way to tell a workflow to use a larger runner involves creating a "runner group", giving that an arbitrary name, and then using that arbitrary name in the runs-on element. Is that appearance accurate with your experience?

If all that's correct, then it seems like the only practical solution is to get the upstream JSON schema relaxed to make runs-on accept any string or array-of-strings, and then pull that into action-validator. If you can confirm my understanding is accurate, I'll open an issue on schemastore to get things moving there.

deviantintegral commented 1 year ago

Yes, I think that describes it. You create your own labels, which can point to runners or runner groups. For example, I created an ubuntu-22.04-l runner. I think you're right about needing to reduce the schema constraints.

mpalmer commented 1 year ago

With further reading and pondering, and a suuuuper-detailed perusal of the entire jobs.<job_id>.runs-on docs, I think that the GitHub-recommended way to choose a custom runner group is something like this:

jobs:
  foo:
    runs-on:
      group: ubuntu-22.04-l

The docs are very misleading about this, as they open by saying that runs-on takes either a string or an array of strings, before (eventually) giving examples where runs-on is a map, as in my example above. I've opened https://github.com/github/docs/issues/26265 to try and get that improved.

In any event, is there any objection to switching your runs-on to use this map-based form, instead (such as, for example, that it doesn't actually work... :grin:)? I rather like the look of this new form, as the mix of labels and group names in a single array-of-strings would seem to me to be rather confusing.

deviantintegral commented 1 year ago

When I switch to the group syntax, I get Unable to find self-hosted runner group: 'ubuntu-22.04-l'., so it looks like that doesn't actually work.

I suppose we could create a runner group, but for GitHub hosted runners that seems like an extra step given they already handle autoscaling within a single larger "runner".

image
mpalmer commented 1 year ago

Oh, so you can label an individual runner, and/or create a runner group (which may also have label(s) on it). So does

jobs:
  foo:
    runs-on:
      labels: ubuntu-22.04-l

work instead?

deviantintegral commented 1 year ago

It does work! Thanks for the suggestion.

mpalmer commented 1 year ago

Thanks for confirming. It feels like this isn't technically a problem with action-validator, so I'll close this off, but if you disagree, please feel free to reopen.