feat: support inspect the model artifact from remote registry#189
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Backend
participant Config
User->>CLI: Run inspect command with flags
CLI->>Config: Parse flags into Inspect config
CLI->>Backend: Call Inspect(ctx, target, inspectConfig)
Backend->>Backend: Use config to determine remote/local/secure fetch
Backend->>Backend: Fetch manifest and model config accordingly
Backend-->>CLI: Return InspectedModelArtifact
CLI-->>User: Output inspection result
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
pkg/config/inspect.go (1)
25-31: Simplify the constructor function.The constructor function works correctly, but it can be simplified since Go's zero values for booleans are already
false.func NewInspect() *Inspect { - return &Inspect{ - Remote: false, - PlainHTTP: false, - Insecure: false, - } + return &Inspect{} }Alternatively, you could consider whether the constructor function is needed at all, since
&config.Inspect{}provides the same result and is more idiomatic in Go.cmd/inspect.go (1)
54-54: Fix error message to match command context.The error message mentions "cache inspect flags" which appears to be a copy-paste error from another command.
- panic(fmt.Errorf("bind cache inspect flags to viper: %w", err)) + panic(fmt.Errorf("bind inspect flags to viper: %w", err))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/inspect.go(3 hunks)pkg/backend/attach.go(7 hunks)pkg/backend/attach_test.go(1 hunks)pkg/backend/backend.go(1 hunks)pkg/backend/inspect.go(2 hunks)pkg/backend/inspect_test.go(2 hunks)pkg/config/inspect.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
pkg/backend/inspect.go (2)
pkg/config/inspect.go (1)
Inspect(19-23)pkg/backend/referencer.go (1)
ParseReference(36-43)
pkg/backend/inspect_test.go (1)
pkg/config/inspect.go (1)
Inspect(19-23)
pkg/backend/attach_test.go (1)
pkg/config/attach.go (1)
Attach(21-29)
pkg/backend/backend.go (2)
pkg/config/inspect.go (1)
Inspect(19-23)pkg/backend/inspect.go (1)
InspectedModelArtifact(32-55)
cmd/inspect.go (1)
pkg/config/inspect.go (2)
NewInspect(25-31)Inspect(19-23)
🪛 GitHub Actions: CI
pkg/backend/inspect_test.go
[error] 137-137: TestInspect failed: expected digest 'sha256:2bc8836f5910ec63a01109e20db67c2ad7706cb19bef5a303bc86fa5572ec9a2' but got 'sha256:9ca701e8784e5656e2c36f10f82410a0af4c44f859590a28a3d1519ee1eea89d'.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (21)
pkg/backend/inspect_test.go (1)
26-27: LGTM - Import changes align with new interface.The import reordering and addition of the
pkgconfigimport correctly supports the newInspectconfiguration struct usage.pkg/backend/backend.go (1)
59-59: LGTM - Interface signature correctly updated for new functionality.The addition of the
cfg *config.Inspectparameter properly extends the interface to support the new remote inspection capabilities. The signature change is consistent with other methods in the interface that accept configuration structs.pkg/backend/attach_test.go (2)
47-47: LGTM - Test correctly adapted to refactored method signature.The change from passing the entire config struct to individual boolean parameters improves clarity and aligns with the refactoring mentioned in the PR objectives.
55-55: LGTM - Consistent parameter passing for error case.The error test case is correctly updated to use the same individual parameter approach, maintaining consistency with the successful test case.
pkg/config/inspect.go (1)
19-23: LGTM - Well-designed configuration struct.The
Inspectstruct clearly defines the three configuration options for remote inspection functionality. The field names are descriptive and follow Go naming conventions.pkg/backend/inspect.go (5)
68-68: LGTM: Method signature updated correctly.The addition of the
cfg *config.Inspectparameter aligns with the PR objective to support configurable inspection behavior for remote registries.
74-77: LGTM: Configuration parameters passed correctly.The call to
getManifestnow correctly passes the configuration flags (cfg.Remote,cfg.PlainHTTP,cfg.Insecure) as explicit boolean parameters, which is consistent with the refactored method signature inattach.go.
79-82: LGTM: Manifest marshaling for digest calculation.The addition of manifest marshaling is necessary for the new digest calculation approach. The error handling is appropriate.
84-87: LGTM: Configuration parameters passed correctly.The call to
getModelConfigcorrectly passes the configuration flags as explicit boolean parameters, maintaining consistency with the refactored method signatures.
91-91: LGTM: Improved digest calculation for remote inspection.The change from store-based digest retrieval to computing the digest from marshaled manifest bytes is more appropriate for remote inspection scenarios where local storage may not be available. This ensures consistent digest calculation regardless of the inspection source.
cmd/inspect.go (3)
31-31: LGTM: Configuration initialization.The
inspectConfigvariable is properly initialized using the constructor function from the config package.
48-52: LGTM: Flag definitions are correct.The new flags are properly defined and bound to the correct configuration fields. The flag names and descriptions clearly convey their purpose for remote inspection capabilities.
69-69: LGTM: Configuration passed to backend method.The
inspectConfigis correctly passed to the backend'sInspectmethod, enabling the new remote inspection functionality.pkg/backend/attach.go (8)
61-64: LGTM: Updated method call with explicit parameters.The call to
getManifestcorrectly passes the explicit boolean parameters from theAttachconfiguration, maintaining the same functionality with improved clarity.
66-69: LGTM: Updated method call with explicit parameters.The call to
getModelConfigcorrectly passes the explicit boolean parameters from theAttachconfiguration, consistent with the refactoring approach.
172-172: LGTM: Method signature refactored for explicit parameters.The
getManifestmethod signature now accepts explicit boolean parameters (fromRemote,plainHTTP,insecure) instead of a config struct, improving API clarity and reducing coupling.
184-184: LGTM: Conditional logic using explicit parameter.The conditional check now uses the explicit
fromRemoteparameter, maintaining the same logic with improved readability.
198-198: LGTM: Remote client creation with explicit parameters.The remote client is created using the explicit
plainHTTPandinsecureparameters, maintaining consistency with the refactored method signature.
217-217: LGTM: Method signature refactored for explicit parameters.The
getModelConfigmethod signature now accepts explicit boolean parameters, consistent with thegetManifestrefactoring approach.
229-229: LGTM: Conditional logic using explicit parameter.The conditional check correctly uses the explicit
fromRemoteparameter, maintaining consistent logic flow.
244-244: LGTM: Remote client creation with explicit parameters.The remote client creation uses the explicit boolean parameters, maintaining consistency with the refactored approach throughout the file.
Signed-off-by: chlins <chlins.zhang@gmail.com>
chlins
force-pushed
the
feat/inspect-remote
branch
from
18e3736 to
a8ef9c9
Compare
…ack#189) Signed-off-by: SAY-5 <say.apm35@gmail.com>
2 participants
This pull request introduces enhancements to the
inspectcommand and refactors related backend methods to improve configurability and maintainability. Key changes include adding a newInspectconfiguration struct, updating theinspectcommand to support additional flags, and refactoring backend methods to use explicit parameters instead of configuration structs.Enhancements to the
inspectcommand:Inspectconfiguration struct inpkg/config/inspect.goto handle options likeRemote,PlainHTTP, andInsecure. A helper functionNewInspectinitializes this struct with default values.cmd/inspect.goto include new flags (--remote,--plain-http,--insecure) for theinspectcommand, binding them to theInspectconfiguration struct. [1] [2]Refactoring backend methods:
Inspectmethod inpkg/backend/backend.goto accept the newInspectconfiguration struct as a parameter. Updated its implementation inpkg/backend/inspect.goto utilize the new configuration struct for handling remote and secure connections. [1] [2]getManifestandgetModelConfigmethods inpkg/backend/attach.goto replace theAttachconfiguration struct with explicit parameters forRemote,PlainHTTP, andInsecure. This improves clarity and reduces dependency on configuration structs. [1] [2]Updates to tests:
pkg/backend/attach_test.goandpkg/backend/inspect_test.goto reflect the new method signatures and configuration struct usage. [1] [2]Summary by CodeRabbit