Address PR #27 feedback: Refactor to use templates, standard client, and add parameter generation by Copilot · Pull Request #28 · equinix/cli

Copilot · 2025-10-16T19:36:18Z

Summary of Changes

All PR #27 feedback has been addressed:

Templates in /templates directory - Created templates/cmd/service.go.tmpl and templates/internal/service.go.tmpl for service onboarding
Use standard client - fabricv4 now uses internal/api/api.NewStandardClient() for authentication
Proper package structure - Moved from internal/api/fabricv4.go to internal/fabricv4/fabricv4.go
Parameter generation - Implemented reflection-based parameter flag generation in register package
Alias support - Added commented stub section for command aliases in template (aliasing vs renaming strategy to be determined in future PR)
No package stuttering - Renamed methods: GetFabricV4Services → GetServices, NewFabricV4Client → NewClient
Consistent error messaging - All commands use simple error messages from internal/api layer. Detailed configuration help will be addressed via a future equinix config subcommand

All tests pass, linting is clean, and CodeQL found no security issues.

Original prompt

address PR #27 feedback

💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

github-actions · 2025-10-16T19:37:29Z

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "Address PR #27 feedback: Refactor to use templates, standard client, and add parameter generation". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

Co-authored-by: displague <317653+displague@users.noreply.github.com>

github-actions · 2025-10-16T19:49:47Z

Uncommitted changes were detected in the docs/ directory. Please run make docs and commit the changes to your branch.

Co-authored-by: displague <317653+displague@users.noreply.github.com>

displague

@copilot just a few comments that we can address here.

displague · 2025-10-17T14:31:44Z

+	default:
+		// If the name is generic, add param index
+		if flagName == "string" || flagName == "int" || flagName == "bool" {
+			return fmt.Sprintf("param-%d", index)


It makes sense that we can't invent a name for unnamed embedded or composite types.

displague · 2025-10-17T14:34:45Z

+		// This validates credentials before execution
+		_, err := {{SERVICE}}.NewClient()
+		if err != nil {
+			fmt.Fprintf(os.Stderr, "Error initializing {{SERVICE_DISPLAY}} client: %v\n", err)


This validation and warning output is currently happening within internal/api/api.go, so this is redundant, but it does make sense to use the cmd/ rather than internal/ for interface I/O (arguments, output, notifications) -- essentially following an MVC pattern. Because internal/api/api.go is returning the warnings today, cmd/api.go does not. We should be consistent between credential utilizing cmds.

Updated cmd/api.go to show consistent, helpful error messages for missing credentials, following the MVC pattern where cmd/ handles user-facing I/O. The error messages now adapt based on the authentication type (standard OAuth2, Metal token, or Portal cookie).

Commit: SHORT_HASH (will be updated after push)

@copilot Let's roll that change back, per @ctreatma:

It's possible credential errors are the only thing happening on client creation, but that may not be true forever, and this will distract from the more specific error output that comes before it. I think it would be better to keep the error output as-is and (1) add a equinix config subcommand for setting up and checking configuration and (2) see if there's a way we can get a more detailed error message out of viper if we request a config entry that isn't set.

If Cobra (or viper, which is pushing the environment or config settings into args) can require a parameter/config/env under certain action/sub-actions (all those that are being generated to depend on the API client), then (2) could be handled automatically by viper/cobra.

We can address (1) in a separate PR at another time.

Reverted the detailed credential error messages. All commands now consistently show simple error messages directly from internal/api. Configuration help will be addressed via a future equinix config subcommand.

Changes:

Reverted cmd/api.go to original simple error handling

Simplified templates/cmd/service.go.tmpl

Simplified cmd/fabricv4.go

Commit: Will update with hash after push

Copilot

Pull Request Overview

This PR refactors the dynamic command registration system for Fabric v4 by introducing proper templates, eliminating code duplication through standard client usage, fixing package naming conventions, and implementing automatic parameter generation for CLI commands.

Key changes:

Created dedicated template files (templates/cmd/service.go.tmpl and templates/internal/service.go.tmpl) for service scaffolding
Refactored authentication to reuse api.NewStandardClient() instead of duplicating OAuth2 setup
Implemented reflection-based parameter generation that automatically creates CLI flags from method signatures

Reviewed Changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
templates/internal/service.go.tmpl	New template for generating service-specific client initialization code
templates/cmd/service.go.tmpl	New template for generating service command structure with command registration
internal/register/register.go	Added reflection-based parameter generation logic to automatically create CLI flags
internal/fabricv4/fabricv4.go	New file containing Fabric v4 client initialization using standard client
internal/api/fabricv4.go	Removed file containing duplicated OAuth2 setup code
go.mod	Removed direct dependency on hashicorp/go-retryablehttp
cmd/fabricv4.go	Updated to use new fabricv4 package and renamed methods to avoid stuttering
Makefile	Updated onboard target to use new template system

Comments suppressed due to low confidence (1)

go.mod:3

Go version 1.24.5 does not exist. The latest stable Go version as of January 2025 is 1.23.x. This should be corrected to a valid Go version such as 1.23.0 or 1.22.0.

go 1.24.5

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-17T14:39:58Z

+	var result strings.Builder
+	for i, r := range typeName {
+		if i > 0 && r >= 'A' && r <= 'Z' {
+			result.WriteRune('-')
+		}
+		result.WriteRune(r)
+	}


The kebab-case conversion logic in generateParamName duplicates the same logic already present in extractMethodName (lines 131-139). Consider extracting this into a shared helper function to avoid code duplication.

Co-authored-by: displague <317653+displague@users.noreply.github.com>

ctreatma · 2025-10-17T18:23:27Z

+			fmt.Fprintln(os.Stderr, "\nPlease ensure the following are set:")
+			if portal {
+				fmt.Fprintln(os.Stderr, "  - EQUINIX_PORTAL_COOKIE environment variable")
+				fmt.Fprintln(os.Stderr, "\nOr in your config file (~/.config/equinix/equinix.yaml):")
+				fmt.Fprintln(os.Stderr, "  equinix_portal_cookie: your-cookie-value")
+			} else if isMetalPath(path) {
+				fmt.Fprintln(os.Stderr, "  - METAL_AUTH_TOKEN environment variable")
+				fmt.Fprintln(os.Stderr, "\nOr in your config file (~/.config/equinix/equinix.yaml):")
+				fmt.Fprintln(os.Stderr, "  metal_auth_token: your-token")
+			} else {
+				fmt.Fprintln(os.Stderr, "  - EQUINIX_CLIENT_ID environment variable")
+				fmt.Fprintln(os.Stderr, "  - EQUINIX_CLIENT_SECRET environment variable")
+				fmt.Fprintln(os.Stderr, "\nOr in your config file (~/.config/equinix/equinix.yaml):")
+				fmt.Fprintln(os.Stderr, "  equinix_client_id: your-client-id")
+				fmt.Fprintln(os.Stderr, "  equinix_client_secret: your-client-secret")
+			}


I'm not a fan of this. It's possible credential errors are the only thing happening on client creation, but that may not be true forever, and this will distract from the more specific error output that comes before it. I think it would be better to keep the error output as-is and (1) add a config subcommand for setting up and checking configuration and (2) see if there's a way we can get a more detailed error message out of viper if we request a config entry that isn't set.

ctreatma · 2025-10-17T18:26:23Z

+	// Example: Add shorter aliases for commonly used commands
+	// fabricv4Cmd.Aliases = []string{"fabric"}
+	//
+	// Or add aliases to specific subcommands after registration:


Can we rename commands instead of aliasing?

ctreatma · 2025-10-17T18:31:05Z

+
+	// Uncomment and customize aliases as needed for convenience
+	// Example: Add shorter aliases for commonly used commands
+	// fabricv4Cmd.Aliases = []string{"fabric"}


Renaming instead of aliasing would be useful for things like this because we probably don't want customers to have to think "do I want fabricv4 or fabricv5?" We want them to focus on Fabric the product, not how the Fabric control plane is currently implemented.

When prompting for this support, I wasn't thinking about the top level commands. I was considering the cobra sub- commands (the cli actions which are just methods of the clients). Having aliases for the top-level API/SDK generated commands make sense to me.

I thought this might be implemented as a map of generated names to a list of friendly custom names that would be a post-generation edit; subsequent make update commands would be non-descructive in most cases because this map could be stored in a one-off file (i.e. cmd/fabric-aliases.go, cmd/internal/fabricv4/aliases.go)

There is some simplicity in having the initial name of action match the package, but I agree equinix fabric connection create (or connection-create ... create-connection?) is a more intuitive experience.

The make register command is currently generating a few variables from the submitted name. Perhaps the unversioned name (removed the trailing v[0-9]+) should be used as the CLI command name and cmd/ filename.

Looking at https://docs.equinix.com/equinix-api/#api-references, Internet Access has a v1 and v2 where this topic can be somewhat concretely considered. This API does not offer the same endpoints behind v1/ as v2/, v2 endpoints in this path are not replacements, they are additive. The code generation as implemented here would be more interested in the Equinix SDK Go naming where we see only https://github.com/equinix/equinix-sdk-go/tree/main/services/eiav2. If the Equinix SDK Go package were to merge IAv1 and IAv2 specs to the produce EIAv2 package, the CLI action could be considered misleading by offering equinix iav2 get-equinix-internet-access-services. This is further complicated by https://docs.equinix.com/api-catalog/internetaccessv2/ and https://docs.equinix.com/api-catalog/internetaccessv1/ being altogether different than what the SDK is using: https://github.com/equinix/equinix-sdk-go/blob/main/Makefile.eiav2#L6. This may not have been the best example to explore. In any case, for IA, we would want to provide generated or aliased names that would make this command closer to equinix ia search (or list, get, which becomes a question of convention in aliasing).

If there were a complete v5 replacement for a v4 service, a major version bump, I think it is fair to assume that we could see incompatible or removed actions while both APIs are made available for a transition period (as we saw with ecx3 and fabricv4). In those cases, a user would want to be selective about fabricv4 or fabricv5 actions. I could see us changing a fabric alias name (as a CLI breaking change) to refer to v5 while v4 support persists for some time, potentially allowing for a config option to override the alias.

This leads me to suspect we will want to break-glass on automation where cmd/ files are concerned. We may want to combine two SDK packages into one command, or define custom aliases. If the command name is an unprefixed version, we can add a versioned alias, or vice-versa, or not at all -- this can be determined when onboarding a service.

That said -- Pros and cons are awash to me. I'm open to whatever you prefer but I think this could be a direction and set of changes we make after merging this and #27.

Co-authored-by: displague <317653+displague@users.noreply.github.com>

Address PR #27 feedback: Refactor to use templates, standard client, and add parameter generation

Initial plan

3f3ee7b

Copilot AI assigned Copilot and displague Oct 16, 2025

Copilot started work on behalf of displague October 16, 2025 19:37 View session

Copilot AI and others added 2 commits October 16, 2025 19:43

Refactor: Move fabricv4 client to internal/fabricv4 and create templates

8d7ba43

Co-authored-by: displague <317653+displague@users.noreply.github.com>

feat: Add parameter generation using reflection in register package

f204629

Co-authored-by: displague <317653+displague@users.noreply.github.com>

chore: Remove unused retryablehttp dependency

aca4d49

Co-authored-by: displague <317653+displague@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Address feedback from PR #27~~ Address PR #27 feedback: Refactor to use templates, standard client, and add parameter generation Oct 16, 2025

Copilot AI requested a review from displague October 16, 2025 19:55

Copilot finished work on behalf of displague October 16, 2025 19:55

displague reviewed Oct 17, 2025

View reviewed changes

Copilot started work on behalf of displague October 17, 2025 14:39 View session

displague requested a review from Copilot October 17, 2025 14:39

Copilot AI reviewed Oct 17, 2025

View reviewed changes

fix: Add consistent credential error messages to cmd/api.go

6fbf310

Co-authored-by: displague <317653+displague@users.noreply.github.com>

Copilot AI requested a review from displague October 17, 2025 14:46

Copilot finished work on behalf of displague October 17, 2025 14:46

ctreatma reviewed Oct 17, 2025

View reviewed changes

Copilot started work on behalf of displague October 17, 2025 19:51 View session

revert: Simplify credential error messages per feedback

ce7d57b

Co-authored-by: displague <317653+displague@users.noreply.github.com>

Copilot finished work on behalf of displague October 17, 2025 19:58

displague approved these changes Oct 18, 2025

View reviewed changes

displague marked this pull request as ready for review October 18, 2025 01:10

displague merged commit de2741a into copilot/implement-dynamic-registration-corba Oct 18, 2025
2 of 8 checks passed

displague deleted the copilot/address-pr-27-feedback branch October 18, 2025 01:11

ctreatma pushed a commit that referenced this pull request Oct 28, 2025

Merge pull request #28 from equinix/copilot/address-pr-27-feedback

464abcc

Address PR #27 feedback: Refactor to use templates, standard client, and add parameter generation

Uh oh!

Conversation

Copilot AI commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Uh oh!

github-actions Bot commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 16, 2025

Uh oh!

displague left a comment

Choose a reason for hiding this comment

Uh oh!

displague Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

displague Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

displague Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

ctreatma Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

ctreatma Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

ctreatma Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

displague Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Copilot AI commented Oct 16, 2025 •

edited

Loading

github-actions Bot commented Oct 16, 2025 •

edited

Loading