Add VCS module publish command - #105
Conversation
paladin-devops
left a comment
There was a problem hiding this comment.
I have some pretty minor feedback, great job adding this command!
| ShortHelp: "Manage private registry modules.", | ||
| LongHelp: heredoc.New(inv.IO).Mustf(` | ||
| The {{ template "mdCodeOrBold" "%s module" }} command group lets you manage | ||
| private registry modules in HCP Terraform and Terraform Enterprise. | ||
| `, version.Name), |
There was a problem hiding this comment.
Good description and short help text. 👍🏻
| if opts.Quiet { | ||
| logger.Debug("Quiet mode enabled, rendering skipped") | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
We should drain the body before returning here. This way, if the server were to have keep-alives enabled, then the TCP connection could be returned to the connection pool, instead of being closed, which is the default behavior of Go's HTTP implementation for undrained response body.
| if opts.Quiet { | |
| logger.Debug("Quiet mode enabled, rendering skipped") | |
| return nil | |
| } | |
| if opts.Quiet { | |
| logger.Debug("Quiet mode enabled, rendering skipped") | |
| io.Copy(io.Discard, resp.Body) | |
| return nil | |
| } |
| @@ -0,0 +1,3 @@ | |||
| kind: ENHANCEMENTS | |||
| body: "Added `tfctl module publish` for publishing VCS-backed private registry modules from existing OAuth or GitHub App connections" | |||
There was a problem hiding this comment.
nit: Present-tense changelog note.
| body: "Added `tfctl module publish` for publishing VCS-backed private registry modules from existing OAuth or GitHub App connections" | |
| body: "tfctl now has `tfctl module publish` for publishing VCS-backed private registry modules from existing OAuth or GitHub App connections." |
| if response.Data.Links.Self != "" { | ||
| result.SelfLink, err = resolvePublishSelfLink(opts.Client.BaseURL, response.Data.Links.Self) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to resolve registry module self link: %w", err) |
There was a problem hiding this comment.
We should add a test to publish_test.go for this error case.
| return fields | ||
| } | ||
|
|
||
| func resolvePublishSelfLink(base *url.URL, self string) (string, error) { |
There was a problem hiding this comment.
I think that we could add some tests to publish_test.go, maybe in a new function TestResolvePublishSelfLink, which just tests this function's paths, rather than doing so implicitly via TestRunPublishOutputFormats.
| }) | ||
| }, | ||
| }) | ||
| opts.Quiet = true |
There was a problem hiding this comment.
Does Quiet need to be true for this test?
brandonc
left a comment
There was a problem hiding this comment.
Hoping to have a conversation about a potential --provider argument and a couple of usability ideas. Everything looks great
| LongHelp: heredoc.New(inv.IO, heredoc.WithPreserveNewlines()).Mustf(` | ||
| The {{ template "mdCodeOrBold" "%s module publish" }} command publishes a private registry module from an existing VCS connection. | ||
|
|
||
| Provide exactly one of {{ template "mdCodeOrBold" "--oauth-token-id" }} or {{ template "mdCodeOrBold" "--github-app-installation-id" }}. |
There was a problem hiding this comment.
What would you say is the best way to identify the token ID that you should use for this command? I used the api command to navigate around this area but I'm wondering if there could be usability gains by supporting a service-provider or oauth client name.
Imagine:
$ tfctl module publish brandonc/terraform-aws-bootstrap --provider githubIf "github" could be used to disambiguate the list of available providers, matching only one, you could then navigate to whichever token we show on the VCS providers screen.
Fallback option:
$ tfctl module publish brandonc/terraform-aws-bootstrap --oauth-token-id ot-...I've also discovered that it's not possible to discover the github app installation ID because that endpoint requires cookie authentication. Can we provide some guidance about how to get the --github-app-installation-id?
| return nil | ||
| } | ||
|
|
||
| type publishRequestEnvelope struct { |
There was a problem hiding this comment.
I'd love if you could provide some comments here about how these types (and the call to client.Do) are a substitute for missing OpenAPI definition -- it could be migrated to a native Client API call eventually and I don't want to lose sight of that.
| } | ||
|
|
||
| if opts.Quiet { | ||
| logger.Debug("Quiet mode enabled, rendering skipped") |
There was a problem hiding this comment.
It's humorous because this can never be logged by tfctl but I like the completeness. Some day there may be a file log or something.
| logger.Debug("Publishing VCS-backed registry module", | ||
| "method", http.MethodPost, | ||
| "path", requestURL.Path, | ||
| "organization", organization, | ||
| "mode", publishingMode, | ||
| ) |
There was a problem hiding this comment.
FYI the method and path are debug logged by the API Client:
10:28:00.836 [DEBUG] tfctl.module publish: Publishing VCS-backed registry module: method=POST path=/api/v2/organizations/bcroft/registry-modules/vcs organization=bcroft mode=branch-based
10:28:00.836 [DEBUG] tfctl.module publish: HTTP request: method=POST url=https://app.staging.terraform.io/api/v2/organizations/bcroft/registry-modules/vcs
| Namespace string `json:"namespace,omitempty"` | ||
| Provider string `json:"provider,omitempty"` | ||
| Status string `json:"status,omitempty"` | ||
| SelfLink string `json:"self_link,omitempty"` |
There was a problem hiding this comment.
I thought it might be useful to add an html_link here if you can derive it from the response so I can ctrl-click and navigate to it after the command runs.
/app/bcroft/registry/modules/private/ORGANIZATION/NAME/PROVIDER and let the server redirect to the version or show the status
While pending, the UI shows:
Waiting for module MODULENAME to become ready…
This page will automatically refresh.
Description
Adds
tfctl module publishfor publishing VCS private registry modules from existing OAuth or GitHub App connections.The command:
go-tfe/v2does not yet provide a method for this endpoint.For repositories that follow the standard
terraform-<provider>-<name>naming convention, HCP Terraform automatically determines the module name and provider. Custom name/provider overrides and repositories requiring different VCS identifier and display-identifier values are outside this initial scope and can usetfctl api.Validation completed:
go test ./internal/commands/module ./internal/commands/root -count=1make gen/screenshotmake binmake checkgo test ./... -racegit diff --checkExample Output
Tag-based publishing dry run:
Branch-based publishing:
PR Checklist
--json— Forces machine-readable output to stdout.--markdown— Forces Markdown output to stdout.--dry-run— Resolves and validates the request without sending a mutation.--quiet— Suppresses successful output and unessential guidance.make gen/screenshot.PCI review checklist