-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Add a new command to set a predefined runner register token and also add a set-token parameter for registration-token API #32878
Conversation
Improper names ....... why And it needs tests |
|
||
subcmdActionsSetRunnerToken = &cli.Command{ | ||
Name: "set-runner-token", | ||
Usage: "Set a new token for a runner to as register token", |
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 isn't "Set a new token" simply "adding a token"?
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.
Because once the new token is added, all old tokens will be invalided. Only one token will be valid. So that I use set
rather than add
.
cmd/actions.go
Outdated
Name: "set-runner-token", | ||
Usage: "Set a new token for a runner to as register token", | ||
Action: runSetActionsRunnerToken, | ||
Aliases: []string{"grt"}, |
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.
What is it? Copied and pasted?
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.
// in: body | ||
// description: set a runner register token instead of generating one. | ||
// type: string | ||
// required: false |
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.
It's really strange that a GET request accepts "set_token" parameter. And it is anti-pattern to make the GET request updates the target.
https://docs.github.com/en/rest/actions/self-hosted-runners?apiVersion=2022-11-28#create-a-registration-token-for-an-organization
Does GitHub do so?
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.
The previous API is GET
but looks like GitHub uses POST
and Github doesn't support a similar feature.
routers/private/actions.go
Outdated
if errors.Is(err, util.ErrNotExist) || (token != nil && !token.IsActive) { | ||
token, err = actions_model.NewRunnerToken(ctx, owner, repo) | ||
var token *actions_model.ActionRunnerToken | ||
if genRequest.PutToken == "" { |
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.
Why you sometimes call it "set token" but sometimes call it "put token"
Name: "token", | ||
Aliases: []string{"t"}, | ||
Value: "", | ||
Usage: "[{token}] - leave empty will generate a new token, otherwise will update the token to database. The token MUST be a 40 digital string containing only [0-9a-zA-Z]", |
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.
Without a tool, it is difficult for end users to generate it.
The whole design should be like this:
./gitea actions generate-runner-token --global # generate and update
./gitea actions generate-runner-token --scope owner/repo # generate and update
./gitea actions generate-runner-token --display-only # display a token only
./gitea actions generate-runner-token --global --set-token the-token-value # set the token to the pre-generated one
./gitea actions generate-runner-token --scope owner/repo --set-token the-token-value # set the token to the pre-generated one
Do not introduce more technical debts
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.
This is my previous solution. I created a new subcommand now because you said generate-runner-token
is conflicted with set-token
(previous put-token).
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.
This is my previous solution. I created a new subcommand now because you said
generate-runner-token
is conflicted withset-token
(previous put-token).
That's not your previous solution. At least I can see huge difference.
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.
The differences:
- we should forbid the
./gitea actions generate-runner-token
without arguments (maybe it is a breaking change, but I believe we should do it, and it is easy for end users to follow) - we should add
--display-only
to help end users to generate the token --set-token
only works with--global
or--scope
(put
itself is an improper name, I do not remember where else we ever used it)
And one more thing, to fix #23703, there could be a better solution, without making users to manually set the tokens: Add an expiry time to the token, and make the token could be re-used before expiration. There could also be a "global permanent token" which is valid forever |
Use env GITEA_RUNNER_REGISTRATION_TOKEN as global runner token #32946 |
Resolve #23703
This PR adds a new gitea actions command to set a runner register token.
It also adds a new parameter
set-token
for the/api/v1/admin/runners/registration-token
API to allow setting a predefined register token.After the PR is merged, you can now use the command line to set a token like below.
or invoking the API like