-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat(runs_on): permit using array of string to select specific runner #1740
feat(runs_on): permit using array of string to select specific runner #1740
Conversation
Your commits aren't singned.
|
19fa51e
to
e21e74f
Compare
Hi, I saw that we could sign with our SSH key. I thought that since I was using my SSH key to clone the repo, the associated commits were automatically signed. But indeed, a bit of configuration was missing... I have redone my commits as indicated in the procedure and force-pushed these two rebased commits with a signature. I would like to ensure that all the changes I have made work correctly before merging them. It is quite complicated to test due to all the internal references to |
🚀 Pull Request Branch was created or updatedThe pull request branch pr/1740 was created or updated. You can try this pull request in your workflows by changing tfaction version to e.g. - uses: suzuki-shunsuke/tfaction/setup@pr/1740 To update the pull request branch again, please run the workflow. |
@NikitaCOEUR You can test this pull request using the version |
Yes thanks I saw that but I couldn't find any time for that now. Maybe next week! |
🚀 Pull Request Branch was created or updatedThe pull request branch pr/1740 was created or updated. You can try this pull request in your workflows by changing tfaction version to e.g. - uses: suzuki-shunsuke/tfaction/setup@pr/1740 To update the pull request branch again, please run the workflow. |
I'm not sure why this error occurred. - run: bash ${{ github.action_path }}/main.sh This error may occur if you run this action in a container, but I guess you didn't do that. |
Hmm .. I just saw a workaround suggested by someone: actions/runner#716 (comment) I'm not sure if that would make sense here? |
Could you allow me to push commits to the feature branch? e.g. aquaproj/aqua-registry#24089
|
So, I had to transfer the ownership of the fork to my user account to be able to activate this option. |
Oh, really. I didn't know that. Anyway, thank you for your update. |
a0f260e Replaced |
🚀 Pull Request Branch was created or updatedThe pull request branch pr/1740 was created or updated. You can try this pull request in your workflows by changing tfaction version to e.g. - uses: suzuki-shunsuke/tfaction/setup@pr/1740 To update the pull request branch again, please run the workflow. |
The purpose of this pull request seems fine. However, I have issues with the Taskfile combined with the execution of my GitHub Actions in a container. I use a Taskfile at the root of my project as a wrapper for Terraform commands. This is used when a task command is executed from the root or in one of the subfolders. But they have implemented a safeguard based on directory ownership; task ascends through parent directories and stops if it does not find a Taskfile or as soon as the owner of the current directory changes compared to the previous one(https://github.com/go-task/task/blob/573949573968cc7056b98e0fdbee1d145ad869a9/taskfile/taskfile.go#L149) ... And this is the case since the containers on GitHub Actions are executed with the root user... Also, the repo is cloned on the runner, and the volume is mounted in the container. When accessing the repo's folders, the root folder of the repo belongs to the user running Docker, but the subfolders belong to the user of the container (root). |
It seems that a workaround exists : actions/checkout#47 (comment) I'll test that tomorrow. |
Thank you for testing. It looks good. |
But I have a workaround for my problem with Taskfile, which I posted in an issue: go-task/task#1683 (comment) However, this problem appears again with the git command. I just opened an issue here: suzuki-shunsuke/github-action-terraform-init#19 |
The issue was solved. So everything is fine, right? |
Do you have some doc to update for this feature ? |
~/repos/src/github.com/suzuki-shunsuke/tfaction-docs main 07:49:57
$ git grep runs_on
docs/config/tfaction-root-yaml.md: runs_on: ubuntu-latest # default is "ubuntu-latest". This is useful to use GitHub Actions Self Hosted Runner for the specific provider |
✅ I'll update the document. |
I can't ensure non-regression without running within a container. I have an issue; I don't know if it's caused by the transition from |
Could you explain the detail of your issue? |
I've already confirmed this pr worked fine without a container. |
No, it's on my end then; the problem only occurs when I run without a container. I get an exit code 2 when running Terraform through my wrapper. But it must be due to the context change. I use the action within a container anyway, so there's no problem for me. |
schema/tfaction-root.json
Outdated
} | ||
} | ||
}, | ||
"Envs": { | ||
"type": "object", | ||
"description": "environment variables" | ||
}, | ||
"RunsOn": { | ||
"type": ["string", "array"], |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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. 🤔
There was a problem hiding this comment.
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"
}
There was a problem hiding this comment.
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"
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. 7290811
Check List
Require signed commits
, so all commits must be signedPermit using array of string to select GitHub runners on which run jobs.
Necessary to select self hosted runners.