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.

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

feature: support runtime init script #1085

Merged
merged 7 commits into from
Oct 26, 2022
Merged

Conversation

VoVAllen
Copy link
Member

@VoVAllen VoVAllen commented Oct 25, 2022

support runtime.init

Signed-off-by: Jinjing.Zhou <[email protected]>
Signed-off-by: Jinjing.Zhou <[email protected]>
@VoVAllen VoVAllen changed the title WIP feature: support runtime init script feature: support runtime init script Oct 25, 2022
@VoVAllen
Copy link
Member Author

@gaocegege @kemingy PTAL

ruleDaemon = "runtime.daemon"
ruleEnviron = "runtime.environ"
ruleMount = "runtime.mount"
ruleInitScript = "runtime.init_script"
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it better to name as runtime.init?

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

ruleDaemon = "runtime.daemon"
ruleEnviron = "runtime.environ"
ruleMount = "runtime.mount"
ruleInitScript = "runtime.init_script"
Copy link
Member

Choose a reason for hiding this comment

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

SGTM

user = "${USER}"
working-directory = "${%[3]s}"
%[4]s
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is too flexible. Currently, we only need the start-after.

Maybe we can switch to a TOML tool to generate it if it becomes more complex in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can refactor this once we need more controls here.

pkg/lang/ir/supervisor.go Show resolved Hide resolved
@gaocegege
Copy link
Member

LGTM

@gaocegege gaocegege closed this Oct 26, 2022
@gaocegege gaocegege reopened this Oct 26, 2022
@gaocegege
Copy link
Member

Sorry, I closed it in accident.

Signed-off-by: Jinjing.Zhou <[email protected]>
@@ -0,0 +1,40 @@
def install_kubectl_kind():
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep it in envd examples or envdlib?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not general enough now. sudo docker network connect kind dpgen2 means only project named as dpgen2 will work

Copy link
Member

Choose a reason for hiding this comment

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

Could we make like this:

def install_kubectl_kind(environment_name):

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to def install_kubectl_kind(env_name). But I still think kind is not a general requirement. Thus I don't think we don't need to put it into envdlib

Signed-off-by: Jinjing.Zhou <[email protected]>
Copy link
Member

@kemingy kemingy left a comment

Choose a reason for hiding this comment

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

/lgtm

@muniu-bot muniu-bot bot added the lgtm label Oct 26, 2022
@muniu-bot
Copy link

muniu-bot bot commented Oct 26, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaocegege, kemingy, VoVAllen

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [VoVAllen,gaocegege,kemingy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gaocegege gaocegege merged commit ac87c76 into tensorchord:main Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants