- Notifications
You must be signed in to change notification settings - Fork913
fix(agent/agentcontainers): generate devcontainer metadata from schema#16881
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
4c2532a to075d291CompareUh oh!
There was an error while loading.Please reload this page.
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.
Awesome! Is quicktype already a dependency or should we add it to our image/nix/etc? Are you adding a Makefile entry later?
Hmm.. Speaking of Makefile, I wonder if we should commit the spec.json so that we can ensure in CI that there are no changes by gen.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -183,7 +184,7 @@ func devcontainerEnv(ctx context.Context, execer agentexec.Execer, container str | |||
| return nil, nil | |||
| } | |||
| meta := make([]DevContainerMeta, 0) | |||
| meta := make([]dcspec.DevContainer, 0) | |||
| if err := json.Unmarshal([]byte(rawMeta), &meta); err != nil { | |||
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.
Suggestion: We could first attempt a strict unmarshal viadec := json.NewDecoder(), dec.DisallowUnknownFields(), if it catches an issue we can log it and fall-back to non-strict unmarshaling.
Not strictly necessary, but may help provide hints down the line why something isn't working.
| @@ -192,7 +193,10 @@ func devcontainerEnv(ctx context.Context, execer agentexec.Execer, container str | |||
| env := make([]string, 0) | |||
| for _, m := range meta { | |||
| for k, v := range m.RemoteEnv { | |||
| env = append(env, fmt.Sprintf("%s=%s", k, v)) | |||
| if v == nil { // *string per spec | |||
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 interpret this as env vark should be removed. Does the spec specify more concretely what null means in this context (or do we have to try it out)?
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.
Unfortunately, no. I think we'll have to either try it out or read the implementation.
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 seems@devcontainer/cli has a poor implementation, from:
"remoteEnv": {"FOO":"bar","NODE_OPTIONS":"","GOPRIVATE":null},
We get this.
NODE_OPTIONS=GOPRIVATE=nullFOO=barI'm not a fan of the null handling here. But if we want to retain a similar behavior, we shouldn't just skip it.
I'd suggest setting it to an empty string when "none", does that sound reasonable? Or do we want to mimic@devcontainer/cli exactly here?
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.
Having anull sneak in is suprising behaviour to me. I'd honestly prefer an empty string, but we can revisit this if it causes compat issues.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
johnstcn commentedMar 12, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I was hoping to avoid adding a new dependency, but I can add it.
I can do that! Depends on how much spec stuff we want lying around in |
True. It's just that without locking in the version, it might at some point be hard to reproduce a working copy if the output ever diverges.
My main concern is having code that's never or rarely run and ends up being incompatible by the time its needed. I suppose we have two options. 1) Keep it download-only and only run it (manually) when we know there's an update, or 2) Commit the spec json and run generate every time. For 1) a CI cron to check if the output/spec changed would be good. And for 2) we may still want to do CI cron, but in general it will ensure our quicktype dependency remains compatible for when it's needed the next time. |
23209fc to2a565b3Compare
I went ahead with option 2! |
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.
Just a minor nit (as well as? re: null handling), but this is looking great! Thanks for improving the codegen.
Approving, as I don't need to re-review.
agent/agentcontainers/dcspec/gen.sh Outdated
| mv -v "${TMPDIR}/${DEST_FILENAME}" "${DEST_PATH}" | ||
| # Add a header so that Go recognizes this as a generated file. | ||
| sed -i '1s/^/\/\/ Code generated by dcspec\/gen.sh. DO NOT EDIT.\n/' "${DEST_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.
This transformation could be done in temp still, before the move.
But unfortunately, this is not cross-platform 😔.
If you want a quick fix and not use another tool, see:a1f5468
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.
Nice callout, thanks!
13a3ddd intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Generated using
glideapps/quicktype. This is unlikely to change often, but checking it into the repo and adding the ability to update when required.