Skip to content

Update to v2 API (using SDK)#203

Merged
chrisghill merged 5 commits into
mainfrom
massdriver-v2-api
May 28, 2026
Merged

Update to v2 API (using SDK)#203
chrisghill merged 5 commits into
mainfrom
massdriver-v2-api

Conversation

@chrisghill
Copy link
Copy Markdown
Member

@chrisghill chrisghill commented May 28, 2026

Summary of changes:

  • cmd/deployment.go — dropped the dead deployments.NewService(mdClient) line and passed provClient.Deployments (the pre-wired service the new provisioning.NewClient() returns) into deployment.UpdateDeploymentStatus.
  • cmd/resource.go — replaced the missing massdriver/client import with massdriver/provisioning, and now use provClient.Resources for both publish and delete.
  • src/resource/main_test.go — consolidated the fake service: struct + both CreateResource and DeleteResource methods live here.
  • src/resource/publish_test.go — removed the stale duplicate method (which set CreateCalled on delete by mistake); fixed the assertion message; fixed the leading-space typo in "resourceName".
  • src/resource/delete_test.go — removed the artifact-era DeleteArtifact method (now lives in main_test.go as DeleteResource); test now exercises the real resource.Delete path.

@chrisghill
Copy link
Copy Markdown
Member Author

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented May 28, 2026

Claude finished @chrisghill's task in 1m 55s —— View job


Code Review

  • Gather context and read changed files
  • Review changed files and diff
  • Post review findings

Overall this is a clean migration — the new provisioning.NewClient() pattern is simpler and the artifactresource rename is consistent throughout. The Aliases: []string{"artifact"} backward-compat touch is a nice detail. A few issues to address:


Bugs

1. Dead field parameter in src/resource/delete.go

The Delete function accepts field but never passes it to svc.DeleteResource:

// src/resource/delete.go:10-16
func Delete(ctx context.Context, svc ResourceService, id, field string) error {
    ...
    return svc.DeleteResource(ctx, id)  // field is silently dropped
}

Either remove the field param from the signature (and update callers), or confirm that the v2 API intentionally drops it. As-is, field is accepted in cmd/resource.go:164 and delete_test.go but goes nowhere. Fix this →

2. schemasFile is never closed in cmd/resource.go

// cmd/resource.go:98-101
schemasFile, err := os.Open(schemasPath)
if err != nil {
    return telemetry.LogError(span, err, "unable to open schemas file")
}
// no defer schemasFile.Close()

The file handle leaks on every successful publish. Add defer schemasFile.Close(). Fix this →


Minor Issues

3. Stale art* variable names in cmd/resource.go

artFilePath (line 62), artName (line 70), and artFile (lines 83–92) are holdovers from the artifact era:

artFilePath, err := cmd.Flags().GetString("file")
artName, err := cmd.Flags().GetString("name")
var artFile *os.File

These are cosmetic but confusing given the package rename. Fix this →

4. Stale flag description in cmd/resource.go:45

resourcePublishCmd.Flags().StringP("schema-file", "s", "../schema-artifacts.json", "Path to artifact schema file")
//                                                                                    ^^^^^^^^^^ still says "artifact"

Fix this →

5. return err instead of return nil at end of both run functions

Both runResourcePublish (line 132) and runResourceDelete (line 170) end with return err where err is guaranteed to be nil by that point. This is harmless but misleading — return nil makes the intent clear. Fix this →


Observation (not blocking)

6. Pre-release SDK version pinned in go.mod

github.com/massdriver-cloud/massdriver-sdk-go v0.2.5-0.20260528215715-7bd593ce4d83

This is a pseudo-version pointing to an unreleased commit. Fine for development, but worth tagging a stable release before merging if CI isn't already blocking on this.


What's working well

  • provisioning.NewClient() returning structured .Resources/.Deployments fields removes the awkward manual NewService wiring
  • Moving from raw []byte to map[string]any for the resource payload is a cleaner API boundary
  • CLI backward compat via Aliases: []string{"artifact"}
  • backend.go log improvements are useful context for debugging
  • Test consolidation in main_test.go — both CreateResource and DeleteResource fake methods in one place

@chrisghill chrisghill merged commit cfcd831 into main May 28, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants