- Notifications
You must be signed in to change notification settings - Fork23
feat: addcoder_script#154
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Seecoder/coder#9287I decided to drop the `agent` portion from the name. It seemed inconsistentwith `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 replacingstartup_script_behavior?
Could we dobehavior=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. Thebehavior=blocking is kinda weird, because there's alsocron.
I could dologin_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 sayscripts but this sayssource. I preferscript 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 userclone 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 displayingcoder_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 fordisplay_name. I am wondering how we will display them in a better way without overcrowding the dashboard.
IMO, acoder_app provides interactive access to a service, but acoder_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!
Closescoder/coder#9615 |
Seecoder/coder#9287
I decided to drop the
agentportion from the name. It seemed inconsistent withcoder_app.Wait for the PR into
coder/coderbefore thoroughly reviewing.