-
Notifications
You must be signed in to change notification settings - Fork 20
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: add coder_script
#154
Conversation
See coder/coder#9287 I decided to drop the `agent` portion from the name. It seemed inconsistent with `coder_app`.
docs/resources/script.md
Outdated
|
||
### Optional | ||
|
||
- `login_before_ready` (Boolean) This option defines whether or not the user can (by default) login to the workspace before it is ready. Ready means that e.g. the script is done and has exited. When enabled, users may see an incomplete workspace when logging in. |
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 do we have this? I guess we're replacing startup_script_behavior
?
Could we do behavior=blocking
?
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 is replacing that. The behavior=blocking
is kinda weird, because there's also cron
.
I could do login_behavior=after_start
, but I'd rather just have it as a bool with the key.
- `run_on_start` (Boolean) This option defines whether or not the script should run when the agent starts. | ||
- `run_on_stop` (Boolean) This option defines whether or not the script should run when the agent stops. | ||
- `schedule` (String) The schedule to run the script on. This is a cron expression. | ||
- `timeout` (Number) Time in seconds until the agent lifecycle status is marked as timed out, this happens when the script has not completed (exited) in the given time. |
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.
Can you document what you set this to if it's a long-running process? Or I guess we want people to ampersand things &
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 going to allow long-running operations that consistently log, so I'll update this.
provider/script.go
Outdated
"icons here: https://github.com/coder/coder/tree/main/site/static/icon. Use a " + | ||
"built-in icon with `data.coder_workspace.me.access_url + \"/icon/<path>\"`.", | ||
}, | ||
"source": { |
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 docs say scripts
but this says source
. I prefer script
tbh, especially if it's supposed to be an inline script as opposed to an existing file.
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.
Hmm, fair. Source could mean like GitHub or something wack, I'll change it back
### Optional | ||
|
||
- `cron` (String) The cron schedule to run the script on. This is a cron expression. | ||
- `icon` (String) A URL to an icon that will display in the dashboard. View built-in icons here: https://github.com/coder/coder/tree/main/site/static/icon. Use a built-in icon with `data.coder_workspace.me.access_url + "/icon/<path>"`. |
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 do we have icon? Do we want to display all of them on the dashboard?
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.
Yes. These scripts will be displayed in the dashboard.
Optional: true, | ||
Description: "This option defines whether or not the script should run when the agent stops.", | ||
}, | ||
"timeout": { |
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.
For cron jobs that use rclone
to do a backup every 5 minutes, will this timeout cancel them? Backup time is usually variable and is a function of activity in the workspace directory. e.g., number of files, size of files, etc.
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.
Yes. You'd want to add a very lengthy timeout in that case, or not have one at all.
Right now, when two CRONs overlap, the prior process won't end.
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.
A few comments about displaying coder_script
resources in the dashboard.
### Required | ||
|
||
- `agent_id` (String) The "id" property of a "coder_agent" resource to associate with. | ||
- `display_name` (String) The display name of the script to display logs in the dashboard. |
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.
Same for display_name
. I am wondering how we will display them in a better way without overcrowding the dashboard.
IMO, a coder_app
provides interactive access to a service, but a coder_script
will not, so I am not convinced about how and if we should display them.
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 have a nice UI mock that displays this!
Closes coder/coder#9615 |
See coder/coder#9287
I decided to drop the
agent
portion from the name. It seemed inconsistent withcoder_app
.Wait for the PR into
coder/coder
before thoroughly reviewing.