fix(cli): add platform readme link to build output#119
fix(cli): add platform readme link to build output#119jyecusch merged 3 commits intonitrictech:mainfrom
Conversation
Also adds a platform target function
📝 WalkthroughWalkthroughAdded an exported PlatformTarget type and ParsePlatformTarget(target string) in cli/internal/platforms/repository.go to parse targets of the form /@ (revision -> int). GetPlatform now uses ParsePlatformTarget (team, platform, revision) and updated format validation/error messages. In cli/pkg/app/suga.go added SugaApp.getPlatformReadmeURL(target, currentTeam string) which uses ParsePlatformTarget to build a public or private platform README path; Build now computes platformURL after Terraform generation and conditionally inserts a platform-setup step when platformURL is present. No other public APIs changed. Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)cli/pkg/app/suga.go (2)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/internal/platforms/repository.go (2)
66-91: Don’t treat public 404 as definitive; try authenticated fetchIf the platform isn’t public (public 404), it may still be accessible to the user via authenticated API. Currently it returns PlatformNotFound without attempting private fetch. This causes false negatives for cross‑team private platforms.
Apply this diff:
// Smart ordering: try public first if the platform team doesn't match current user's team if team != r.currentTeam { // Try public access first publicPlatformSpec, publicErr := r.apiClient.GetPublicPlatform(team, platform, revision) if publicErr == nil { return publicPlatformSpec, nil } - - // If public fails with 404, it's definitely not found - if errors.Is(publicErr, api.ErrNotFound) { - return nil, terraform.ErrPlatformNotFound - } - - // If public fails for other reasons, try authenticated access + // If public fails (including 404), try authenticated access (resource may be private) platformSpec, err := r.apiClient.GetPlatform(team, platform, revision) if err != nil { if errors.Is(err, api.ErrNotFound) { return nil, terraform.ErrPlatformNotFound } if errors.Is(err, api.ErrUnauthenticated) { return nil, terraform.ErrUnauthenticated } return nil, err } return platformSpec, nil }
93-120: Fix error mapping on fallback and remove unreachable duplicate checkWhen team matches current team:
- If the initial GetPlatform error is NotFound, and public fetch fails with a non‑404, returning ErrUnauthenticated is incorrect. Return the public error (or preserve NotFound mapping).
- The subsequent 404 check is unreachable for NotFound due to the earlier branch.
Refactor as below to map errors correctly and avoid duplication.
// Try authenticated access first platformSpec, err := r.apiClient.GetPlatform(team, platform, revision) if err != nil { - // If authentication failed, try public platform access - if errors.Is(err, api.ErrUnauthenticated) || errors.Is(err, api.ErrNotFound) { - publicPlatformSpec, publicErr := r.apiClient.GetPublicPlatform(team, platform, revision) - if publicErr != nil { - // If public access also fails with 404, return platform not found - if errors.Is(publicErr, api.ErrNotFound) { - return nil, terraform.ErrPlatformNotFound - } - // Return the original authentication error for other public access failures - return nil, terraform.ErrUnauthenticated - } - return publicPlatformSpec, nil - } - - // If its a 404, then return platform not found error - if errors.Is(err, api.ErrNotFound) { - return nil, terraform.ErrPlatformNotFound - } - - // return the original error to the engine - return nil, err + switch { + case errors.Is(err, api.ErrUnauthenticated): + // Fallback to public if auth failed + publicSpec, publicErr := r.apiClient.GetPublicPlatform(team, platform, revision) + if publicErr == nil { + return publicSpec, nil + } + // If not public or other failure, keep unauthenticated semantics + if errors.Is(publicErr, api.ErrNotFound) { + return nil, terraform.ErrUnauthenticated + } + return nil, terraform.ErrUnauthenticated + case errors.Is(err, api.ErrNotFound): + // Try public in case it exists publicly + publicSpec, publicErr := r.apiClient.GetPublicPlatform(team, platform, revision) + if publicErr == nil { + return publicSpec, nil + } + if errors.Is(publicErr, api.ErrNotFound) { + return nil, terraform.ErrPlatformNotFound + } + // Preserve the public error context for non-404 failures + return nil, publicErr + default: + // return the original error to the engine + return nil, err + } } return platformSpec, nil
🧹 Nitpick comments (2)
cli/pkg/app/suga.go (1)
442-460: Doc/behavior mismatch and URL completenessThe comment says “README page,” but the URL omits a readme path or revision. If the README is revision‑specific or lives under a subpath (e.g., /readme), this may link to a generic platform page. Verify the intended path and whether revision should be included.
cli/internal/platforms/repository.go (1)
28-55: Clarify doc and minor parsing nits
- Comment says “Returns nil if the format is invalid” but the function returns an error; clarify to “returns an error.”
- Optional: precompile the regex as a package var to avoid recompilation on each call.
Apply this minimal doc tweak:
-// ParsePlatformTarget parses a platform target string in the format <team>/<platform>@<revision> -// Returns nil if the format is invalid +// ParsePlatformTarget parses a platform target string in the format <team>/<platform>@<revision>. +// Returns a non-nil error if the format is invalid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cli/internal/platforms/repository.go(2 hunks)cli/pkg/app/suga.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cli/pkg/app/suga.go (2)
cli/internal/platforms/repository.go (1)
ParsePlatformTarget(30-54)cli/internal/api/teams.go (1)
Team(8-16)
cli/internal/platforms/repository.go (3)
cli/internal/api/models.go (1)
Platform(70-79)cli/internal/version/version.go (1)
CommandName(13-13)engines/terraform/platform.go (1)
PlatformSpec(41-54)
🔇 Additional comments (2)
cli/pkg/app/suga.go (1)
22-22: Import looks goodNew dependency is used by getPlatformReadmeURL.
cli/internal/platforms/repository.go (1)
21-27: Struct addition LGTMPlatformTarget fields and export scope look appropriate.
|
🎉 This PR is included in version 0.1.27 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
6 participants
Also adds a platform target function