fix(agent/agentcontainers): generate devcontainer metadata from schema #16881
Conversation
4c2532a to
075d291
Compare
| @@ -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.
Suggestion: We could first attempt a strict unmarshal via dec := 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.
I interpret this as env var k 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.
Unfortunately, no. I think we'll have to either try it out or read the implementation.
There was a problem hiding this comment.
It seems @devcontainer/cli has a poor implementation, from:
"remoteEnv": {
"FOO": "bar",
"NODE_OPTIONS": "",
"GOPRIVATE": null
},We get this.
NODE_OPTIONS=
GOPRIVATE=null
FOO=bar
I'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.
Having a null sneak in is suprising behaviour to me. I'd honestly prefer an empty string, but we can revisit this if it causes compat issues.
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 to
2a565b3
Compare
I went ahead with option 2! |
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.
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
Generated using
glideapps/quicktype. This is unlikely to change often, but checking it into the repo and adding the ability to update when required.