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.

Sign up for GitHub

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

Jump to bottom

feat(runs_on): permit using array of string to select specific runner #1740

Merged
merged 6 commits into from
Jul 19, 2024
Merged

feat(runs_on): permit using array of string to select specific runner #1740

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
891baef
feat(runs_on): permit using array of string to select specific githug…
NikitaCOEUR Jul 8, 2024
e21e74f
docs(schema): add runs_on to schema
NikitaCOEUR Jul 8, 2024
560de74
fix(runs_on): ensure runs_on can be optional as before
NikitaCOEUR Jul 15, 2024
a0f260e
fix: replace github.action_path with GITHUB_ACTION_PATH
suzuki-shunsuke Jul 16, 2024
deb247e
chore: update terraform-fmt and terraform-init actions
suzuki-shunsuke Jul 17, 2024
7290811
fix: update JSON Schema of tfaction-root.yaml to use anyOf for runs_on
suzuki-shunsuke Jul 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
docs(schema): add runs_on to schema
  • Loading branch information
@NikitaCOEUR
NikitaCOEUR committed Jul 8, 2024
commit e21e74f655dfe75ea5c1169941717d21b2ffb49b
10 changes: 10 additions & 0 deletions schema/tfaction-root.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@
"providers_lock_opts": {
"$ref": "#/$defs/TerraformProvidersLockOptions"
},
"runs_on": {
"$ref": "#/$defs/RunsOn"
},
"s3_bucket_name_tfmigrate_history": {
"$ref": "#/$defs/S3BucketNameTfmigrateHistory"
},
Expand Down Expand Up @@ -398,12 +401,19 @@
},
"envs": {
"$ref": "#/$defs/Envs"
},
"runs_on": {
"$ref": "#/$defs/RunsOn"
}
}
},
"Envs": {
"type": "object",
"description": "environment variables"
},
"RunsOn": {
"type": ["string", "array"],
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with JSON Schema, so I'm not sure if this is correct.
I'll look into.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://cswr.github.io/JsonSchema/spec/multiple_types/
It seems correct, no ?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 type can accept an array.

https://json-schema.org/draft/2020-12/json-schema-core

"type": ["object", "boolean"],

But I think array is ambiguous because the type of elements is unknown. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is better :

"RunsOn": {
  "oneOf": [
    {
      "type": "string"
    },
    {
      "type": "array",
      "items": {
        "type": "string"
      }
    },
    {
      "type": "null"
    }
  ],
  "description": "The type of runner that the job will run on"
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://json-schema.org/understanding-json-schema/reference/combining#anyOf

Should we use anyOf?

    "RunsOn": {
      "anyOf": [
        {
          "type": "string"
        },
        {
          "type": "array",
          "items": {
            "type": "string"
          }
        }
      ],
      "description": "The type of runner that the job will run on"
    }

Copy link
Contributor Author

@NikitaCOEUR NikitaCOEUR Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading more, Anyof seems more appropriate. It's more like a OR and not a XOR. So yes definitely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But null type is necessary because we can let empty the property no?

Copy link
Owner

@suzuki-shunsuke suzuki-shunsuke Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If runs_on is not set, the type definition is unrelated. So I don't think we need to add null.
If we want to support the setting which sets null explictly, we need to add null.
But I don't think we need to support null explicitly.

runs_on: null

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. 7290811

"description": "The type of runner that the job will run on"
}
}
}