feat: ai rle v1#8782
Conversation
|
Thank you for your contribution @farhann1! We will review the pull request and get back to you soon. |
|
@farhann1 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
Pull request overview
Adds a new first-party azure.ai.rle azd extension scaffold, establishing the initial command surface area and packaging/build plumbing so the extension can be built, packed, and tested prior to implementing the real RLE workflow.
Changes:
- Introduces the
azd ai rlecommand group withcreate,modify,version, and hiddenmetadatacommands (stubbed with structured “not implemented” errors). - Adds extension packaging metadata (
extension.yaml,version.txt,CHANGELOG.md) plus docs and lint/spellcheck configs for the new module. - Adds cross-platform build scripts and the Go module (
go.mod/go.sum) for the extension.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cli/azd/extensions/azure.ai.rle/version.txt | Declares initial extension version. |
| cli/azd/extensions/azure.ai.rle/README.md | Documents commands and private testing workflow. |
| cli/azd/extensions/azure.ai.rle/main.go | Extension entrypoint wiring azdext.Run to the root command. |
| cli/azd/extensions/azure.ai.rle/internal/cmd/root.go | Root command setup and subcommand registration. |
| cli/azd/extensions/azure.ai.rle/internal/cmd/create.go | Adds create command stub + shared not-implemented error helper. |
| cli/azd/extensions/azure.ai.rle/internal/cmd/modify.go | Adds modify command stub. |
| cli/azd/extensions/azure.ai.rle/internal/cmd/version.go | Adds version command wiring for standard version output. |
| cli/azd/extensions/azure.ai.rle/internal/cmd/metadata.go | Adds hidden metadata command for discovery/IntelliSense. |
| cli/azd/extensions/azure.ai.rle/internal/cmd/root_test.go | Basic test asserting expected commands are registered. |
| cli/azd/extensions/azure.ai.rle/go.mod | New Go module for the extension and dependency set. |
| cli/azd/extensions/azure.ai.rle/go.sum | Dependency checksums for the new module. |
| cli/azd/extensions/azure.ai.rle/extension.yaml | Extension manifest (id/namespace/usage/examples/version). |
| cli/azd/extensions/azure.ai.rle/cspell.yaml | Extension-local cspell configuration. |
| cli/azd/extensions/azure.ai.rle/CHANGELOG.md | Initial changelog entry for the preview scaffold. |
| cli/azd/extensions/azure.ai.rle/build.sh | Bash build script for multi-platform binaries + ldflags version injection. |
| cli/azd/extensions/azure.ai.rle/build.ps1 | PowerShell build script for multi-platform binaries + ldflags version injection. |
| cli/azd/extensions/azure.ai.rle/.golangci.yaml | Extension-local lint config (matching other extensions’ pattern). |
| cli/azd/extensions/azure.ai.rle/.gitignore | Ignores build and packaging artifact directories. |
| Write-Host "Error: Failed to get git commit hash" | ||
| exit 1 | ||
| } | ||
| $BUILD_DATE = (Get-Date -Format "yyyy-MM-ddTHH:mm:ssZ") |
jongio
left a comment
There was a problem hiding this comment.
Review: azure.ai.rle extension (new)
Thanks for contributing a new extension! A few items to address before this is ready.
Overview
This adds a new azd ai rle command group for managing RLE (Reinforcement Learning Environment) resources. The implementation is clean and follows the extension SDK patterns well. Tests pass and it builds cleanly.
Key items
-
Missing PR description. A 2949-line new extension needs a description explaining what RLE is, the motivation, the commands it adds, and any design decisions. Right now the body is empty.
-
Unreachable code. Five files define commands (
create,list,show,sandbox,versions) that are never registered inroot.go. These add ~500 lines of untestable surface area. Either wire them up (if intended for this PR) or remove them and add in a follow-up when they're needed. -
Cross-platform line endings.
scaffold.go:67unconditionally converts\nto\r\n. Generated Dockerfiles and Python files will have Windows line endings on Linux/macOS, which can cause issues with container builds and shebangs. -
Unquoted paths in user-facing output.
init.go:55printscd %swithout quoting. Paths containing spaces will break the suggested command. This repo's conventions require quoting paths (see AGENTS.md). -
Bearer token over HTTP. The client sends
RLE_BEARER_TOKENregardless of the endpoint scheme. Consider logging a warning (or refusing) when sending credentials over non-TLS connections to non-localhost endpoints.
See inline comments for specifics.
| rootCmd.AddCommand(newMetadataCommand(rootCmd)) | ||
|
|
||
| return rootCmd | ||
| } |
There was a problem hiding this comment.
Five command constructors (newCreateCommand, newListCommand, newShowCommand, newSandboxCommand, newVersionsCommand) are defined in this package but never called. If they're planned for a follow-up PR, consider removing them from this PR to keep the diff focused. If they should be available now, wire them in here.
| content = strings.ReplaceAll(content, token, value) | ||
| } | ||
| content = strings.ReplaceAll(content, "\n", "\r\n") | ||
| fullPath := filepath.Join(sessionDir, filepath.FromSlash(relativePath)) |
There was a problem hiding this comment.
This forces Windows line endings on all platforms. Generated Dockerfiles and Python files will have \r\n on Linux/macOS, which can cause container build failures (e.g., shebang parsing) and git noise.
Remove this line entirely. Let files keep their native \n endings. Git handles CRLF conversion on Windows if .gitattributes is configured.
| _, err = fmt.Fprintf( | ||
| cmd.OutOrStdout(), | ||
| "Created OpenEnv-style environment at: %s\nNext steps:\n cd %s\n azd ai rle deploy\n", | ||
| displayDir, |
There was a problem hiding this comment.
Paths with spaces will break the suggested cd command. This repo requires quoting user-facing paths (AGENTS.md convention). Use %q instead of %s for the cd target.
| if body != nil { | ||
| req.Header.Set("Content-Type", "application/json") | ||
| } | ||
| if token := os.Getenv("RLE_BEARER_TOKEN"); token != "" { |
There was a problem hiding this comment.
Consider logging a warning (to stderr) when sending a bearer token over a non-TLS, non-localhost endpoint. Silent credential leakage over HTTP to a remote host is a security concern.
jongio
left a comment
There was a problem hiding this comment.
Incremental review (commit d621e52)
New deploy enhancements look well-structured. The splitImageHost/normalizeRegistryLoginServer composition is clean, and using errors.AsType for the 404 check is idiomatic Go 1.26. A few items:
Previous review items (5 findings from my earlier pass) are still unresolved: unreachable command constructors, cross-platform line endings in scaffold.go, unquoted paths in init.go, bearer token over HTTP. Please address those alongside the new work.
| } | ||
|
|
||
| // normalizeRegistryLoginServer turns a registry short name ("devrle") into a login server | ||
| // ("devrle.azurecr.io"). A value that already contains a '.' is assumed to be a login server. |
There was a problem hiding this comment.
These pure helper functions (splitImageHost, normalizeRegistryLoginServer, registryShortName, fileExists) plus isNotFoundError in client.go are great candidates for table-driven unit tests. They have clear input/output contracts and no side effects (except fileExists). Consider adding coverage in a follow-up, especially for edge cases like an image with no tag, an empty registry string, or a login server without a dot.
| registry string | ||
| skipBuild bool | ||
| }{} | ||
|
|
There was a problem hiding this comment.
[nit] The default registry value 'devrle' is clearly a dev-team convenience. That is fine for now, but document it in the --registry help string or README so users outside the team know they need to override it.
| return &azdext.LocalError{ | ||
| Message: fmt.Sprintf("Failed to build and push image '%s' to registry '%s': %v", repoTag, registryName, err), | ||
| Code: "rle_acr_build_failed", | ||
| Category: azdext.LocalErrorCategoryUser, |
There was a problem hiding this comment.
The 404-fallback-to-create is a nice UX touch. One consideration: if state.Project was accidentally changed (or the state file was corrupted), this silently creates a new environment rather than surfacing the mismatch. The printed message helps, but you might want to log the old environment ID in the output so users can spot unintentional recreation.
jongio
left a comment
There was a problem hiding this comment.
Incremental review (commits 9dc600d, 6772f58)
The invoke implementation is taking shape. The approach of fetching Loom via shallow clone plus bundled wheel is reasonable for a dev workflow. A few items need attention before this can merge.
Prior review items: 3 of 5 findings from my earlier passes remain unresolved (unreachable command constructors in root.go, cross-platform CRLF in scaffold.go, unquoted path in init.go). The bearer-token item was addressed by removal.
New findings (this commit):
-
Binary wheel without provenance (
assets/rle_sdk-0.1.3-py3-none-any.whl): Committing a pre-built.whlmakes the package un-auditable. Add a README or comment documenting where this wheel is built, what source repo/tag it corresponds to, and how a reviewer can reproduce it. Alternatively, reference a published package feed so CI can fetch it at build time instead of vendoring the binary. -
Deploy ignores stored endpoint (
deploy.go):resolveControlPlaneEndpoint("")replacedresolveControlPlaneEndpoint(state.Endpoint). If a user had an endpoint saved in.azd-rle.jsonfrom the manifest, it's now silently ignored. Was this intentional? If so, note it in the changelog. -
Fragile TOML patching (
invoke.go:ensureLoomLocalSources): String-contains checks + append-to-EOF doesn't guarantee the new lines land inside the[tool.uv.sources]section when one already exists elsewhere in the file. Consider using a TOML library or at minimum insert after the existing header line rather than at EOF. -
Most training flags are not exposed: Only 4 of ~15 parameters have CLI flags. The rest can't be overridden without editing source. Expose them or document why they're fixed.
-
ADO clone URL may require auth:
defaultLoomRecipeRepopoints tomsdata.visualstudio.com. Will this work without credentials for users outside the org? If auth is needed, surface a clear error message.
deploy doesn't need init anymore
jongio
left a comment
There was a problem hiding this comment.
The new commit (a95e826) reworks image resolution to derive per-environment image names from the environment name, with --image as an explicit override on both init and deploy. The self-bootstrapping deploy (works without prior init when rle.yaml exists) is a nice UX improvement. The errors.AsType usage and azdext.LocalError error handling are correct and idiomatic.
Three items from my earlier reviews remain unresolved:
-
scaffold.go:74
strings.ReplaceAll(content, "\n", "\r\n")forces Windows line endings on all platforms. Generated Dockerfiles and Python files will carry\r\ninto Linux containers, which can break shebang parsing and cause git diff noise on non-Windows hosts. Remove this line; let files keep native\nendings. -
init.go:57
cd %sshould use%qto quote paths that may contain spaces (per the repo's path safety convention). -
root.go Five command source files (
create.go,list.go,show.go,sandbox.go,versions.go) are included in the PR but their constructors are never registered inNewRootCommand. If planned for a follow-up, remove them from this PR to reduce the diff.
| for token, value := range replacements { | ||
| content = strings.ReplaceAll(content, token, value) | ||
| } | ||
| content = strings.ReplaceAll(content, "\n", "\r\n") |
There was a problem hiding this comment.
Still present from prior review: this forces \r\n on every generated file regardless of host OS. Dockerfiles and Python scripts with \r bytes can break shebang parsing and cause exec format error inside Linux containers. Remove this line entirely.
| } | ||
| _, err = fmt.Fprintf( | ||
| cmd.OutOrStdout(), | ||
| "Created OpenEnv-style environment at: %s\nNext steps:\n cd %s\n azd ai rle deploy\n", |
There was a problem hiding this comment.
Still present from prior review: the cd path isn't quoted. A path with spaces (e.g. C:\Users\My Name\code_rl) will break the suggested command. Use %q instead of %s for the cd target.
jongio
left a comment
There was a problem hiding this comment.
New commit (f3ab2d2) restores deploy changes lost in the merge conflict. The ACR build integration and image host-qualification helpers work correctly for the described use case.
One new finding: az acr build is called without verifying that az is installed (see inline comment). The invoke command checks for git and uv with exec.LookPath before calling them; the same pattern should apply here.
Three items from earlier reviews remain unresolved:
-
scaffold.go:74
strings.ReplaceAll(content, "\\n", "\\r\\n")forces CRLF on all platforms. Dockerfiles and Python files with\r\nwill break shebang parsing in Linux containers. Remove this line. -
init.go:57
cd %sneeds%qto quote paths with spaces (AGENTS.md convention). -
root.go Five command source files (
create.go,list.go,show.go,sandbox.go,versions.go) define constructors never called fromNewRootCommand. Wire them in or remove them from this PR.
| repoTag, registryName); err != nil { | ||
| return err | ||
| } | ||
| build := exec.CommandContext(cmd.Context(), |
There was a problem hiding this comment.
Add exec.LookPath("az") before this block, matching the git and uv checks in invoke.go. Without it, a missing Azure CLI produces an error about "push access to the registry" when the real problem is that az isn't on PATH.
No description provided.