Skip to content

Launchable Create Cli#357

Open
tylerfong wants to merge 5 commits intomainfrom
launchable-create-cli
Open

Launchable Create Cli#357
tylerfong wants to merge 5 commits intomainfrom
launchable-create-cli

Conversation

@tylerfong
Copy link
Copy Markdown
Contributor

No description provided.

@tylerfong tylerfong requested a review from a team as a code owner April 17, 2026 20:32
@tylerfong tylerfong changed the title Launchable create cli Launchable Create Cli Apr 17, 2026
var script string
switch {
case opts.LifecycleFilePath != "":
content, err := os.ReadFile(opts.LifecycleFilePath)
Copy link
Copy Markdown
Contributor

@patelspratik patelspratik Apr 17, 2026

Choose a reason for hiding this comment

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

can we check filesize before we read it into memory?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

return assignments, nil
}

func validateTunnelName(name string) string {
Copy link
Copy Markdown
Contributor

@patelspratik patelspratik Apr 17, 2026

Choose a reason for hiding this comment

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

could these just be errors?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is just message function for an error call

if start < 1 || start > 65535 || end < 1 || end > 65535 {
return 0, 0, breverrors.NewValidationError(fmt.Sprintf("port range %q must stay within 1-65535", raw))
}
if start >= end {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can't they be equal if you want to use a single port?

Copy link
Copy Markdown
Contributor Author

@tylerfong tylerfong Apr 17, 2026

Choose a reason for hiding this comment

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

That's if it uses the notation "-",implying range, otherwise just specify "22" directly,

Comment thread pkg/cmd/launchable/create.go Outdated
)

const (
defaultLaunchableViewAccess = "public"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

defaulting to public feels scary in case people are testing and using keys in them. How do you feel about defaulting to organization?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

return u.String()
}

func parseSizeToGiB(raw string) (int, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is existing logic you can follow. @drewmalin did some refactor work to try and fix this area because it was wrong in a bunch of places:

Claude recommendation from analyzing cloud SDK and dev-plane:

Recommended fix (smallest path): drop the base-10 aliases and just handle kib/mib/gib/tib (and optionally b), matching what units.Base2Bytes emits. The case-insensitive match is fine to keep for defensiveness.

Better path (longer-term alignment): have the CLI consume the new MinSizeBytes / MaxSizeBytes JSON (cloudv1.Bytes with {value, unit} shape), and use cloudv1.ByteCountInUnitInt64(cloudv1.Gibibyte) directly. Removes string parsing entirely and stays correct across unit changes.

@patelspratik
Copy link
Copy Markdown
Contributor

can you generate some tests?

Comment thread pkg/store/launchable.go
YamlString string `json:"yamlString,omitempty"`
}

type ValidateDockerComposeResponse map[string]interface{}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

doesn't seem like we need the response? Could ValidateDockerCompose just return error, and we just use the res inside the function to se if it isError() ?

for i := range allInstanceTypes.AllInstanceTypes {
if allInstanceTypes.AllInstanceTypes[i].Type == instanceType {
if len(allInstanceTypes.AllInstanceTypes[i].WorkspaceGroups) == 0 {
return nil, breverrors.NewValidationError(fmt.Sprintf("instance type %q does not have a workspace group for this organization", instanceType))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we change this error message to:

instance type %q not valid for this organization since workspace groups aren't a thing users can do anything with

Comment thread pkg/cmd/launchable/doc.md

Use Compose mode when you want:

- a multi-service environment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should this also include jupyter since we set that to true by default for compose?

Comment thread pkg/cmd/launchable/doc.md

Allowed values:

- `public`
Copy link
Copy Markdown
Contributor

@patelspratik patelspratik Apr 17, 2026

Choose a reason for hiding this comment

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

is it worth explaining these visibilities ? e.g. what's the difference between public and published? Are you thinking that's more of a concern for the skill?

if len(name) > 63 {
return "Name must be 63 characters or fewer"
}
if !regexp.MustCompile(`^[a-z0-9]`).MatchString(name) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we pull these three regexes up as vars that way we don't have to compile them every time? like githubDirectoryURLPattern

@brycelelbach
Copy link
Copy Markdown

Hey @tylerfong — I was working on a parallel CLI path (#364) before I noticed this PR. Yours is further along and more comprehensive, so I'm going to close #364 and defer to this one.

Two follow-up asks I'd love to see (happy to do them as separate PRs after this lands):

  1. --from-file <spec.json> as an alternative input mode. The flag-based UX is great for humans, but there's real value in a spec-file path for:

    • CI/automation (programmatically generated launchables, e.g. our NVIDIA accelerated-computing content pipeline),
    • Round-tripping a launchable captured from the Console's DevTools POST /v2/launchables body,
    • Source-controlling launchable definitions alongside the code they deploy.

    The cleanest wire-up would be --from-file as a mutually-exclusive alternative to the individual flags — everything gets parsed into the same CreateEnvironmentLaunchableRequest, and the downstream validation (instance-type availability, compose YAML validation, port-conflict checks) still runs.

  2. Publish a JSON schema for the launchable config. Once --from-file exists, launchables become a first-class artifact type and we should treat them like one: ship a launchable.schema.json (either under docs/ in this repo, or — better — served from the control plane at a stable URL) so that:

    • Editors can do completion/validation against $schema links in spec files,
    • The CLI can validate locally before the round-trip to the server,
    • Launchables can expose their own configs — a launchable that embeds the schema it conforms to is self-describing, which matters for marketplace/discovery UX.

Related bonus ask for the Console team (not this PR, just flagging): once the schema exists, the Console's launchable-creation wizard should accept a JSON upload as an alternative to clicking through the form. Same schema, two entry points (wizard + upload), one source of truth. That closes the loop for users who author their launchable in a text editor and don't want to re-enter everything in a UI.

Happy to drive either (1) or (2) after #357 merges. None of this is a blocker on the existing review.

@drewmalin
Copy link
Copy Markdown
Contributor

Steph actually has been messing with the instance type service proto, and in particular the "get instance types available" route is... available!

We should be able to do:

	req := connect.NewRequest(&devplaneapiv1.ListInstanceTypeRequest{
		Options: &devplaneapiv1.ListInstanceTypeOptions{
			OrganizationId: orgID,
		},
	})

	resp, err := client.ListInstanceType(ctx, req)

and have the full types here. Then we don't need to redefine structs like GPU, Price, Storage, etc. etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants