Add option --json for gh variable list

[w1mvy]

Fixes #8492

$ go run cmd/gh/main.go variable list --help
List variables on one of the following levels:
- repository (default): available to GitHub Actions runs or Dependabot in a repository
- environment: available to GitHub Actions runs for a deployment environment in a repository
- organization: available to GitHub Actions runs or Dependabot within an organization

For more information about output formatting flags, see `gh help formatting`.

USAGE
  gh variable list [flags]

FLAGS
  -e, --env string        List variables for an environment
  -q, --jq expression     Filter JSON output using a jq expression
      --json fields       Output JSON with the specified fields
  -o, --org string        List variables for an organization
  -t, --template string   Format JSON output using a Go template; see "gh help formatting"

INHERITED FLAGS
      --help                     Show help for command
  -R, --repo [HOST/]OWNER/REPO   Select another repository using the [HOST/]OWNER/REPO format

LEARN MORE
  Use `gh <command> <subcommand> --help` for more information about a command.
  Read the manual at https://cli.github.com/manual

# execute on my fork repo
$ go run cmd/gh/main.go variable list --json name (git)-[feat/variable-list-json-output]   [~/go/src/github.com/w1mvy/cli]
[]

[andyfeller]

@w1mvy : thank you for opening this PR! ❤️ At first glance, I'm reminded about similarities when support for exporting secrets was done in #8081 regarding differences in variables at various levels.

So, just trying to correlate that discussion with respect here as it should largely apply.

[andyfeller]

@w1mvy : thank you for opening this PR and improving the GitHub CLI! ❤️

Generally these changes look good with a few suggestions to bring this into alignment with other changes.

[andyfeller]

Could explain why this is needed?

https://github.com/rajhawaldar/cli/blob/976b84afd2ce02bb575df1269baac33a0618ed9b/pkg/cmd/secret/list/list.go#L147-L149

[w1mvy]

Thanks your review!!

Added to make the return value an empty array. example pr list issue list

cli/pkg/cmd/pr/list/list.go

Lines 179 to 181 in 586eb8b

	if len(listResult.PullRequests) == 0 && opts.Exporter == nil {
	return shared.ListNoResults(ghrepo.FullName(baseRepo), "pull request", !filters.IsDefault())
	}

$ gh pr list --json id
[]

However, secret list does not return an empty array.

secret list and variable list have similar usage scenarios, it would be better to align their behavior with that of secret list.

$ gh secret list --json name
no secrets found

[andyfeller]

Think this isn't needed given the default behavior of exporting structs except we will need to add JSON tags to the remaining Variable fields for their lowercased versions so they are exported with the appropriate names.

cli/pkg/cmd/variable/list/list.go

Lines 174 to 181 in b0da755

	type Variable struct {
	Name string
	Value string
	UpdatedAt time.Time `json:"updated_at"`
	Visibility shared.Visibility
	SelectedReposURL string `json:"selected_repositories_url"`
	NumSelectedRepos int
	}

type Variable struct {
	Name             string            `json:"name"`
	Value            string            `json:"value"`
	UpdatedAt        time.Time         `json:"updated_at"`
	Visibility       shared.Visibility `json:"visibility"`
	SelectedReposURL string            `json:"selected_repositories_url"`
	NumSelectedRepos int               `json:"num_selected_repos"`
}

[andyfeller]

Suggested change

	func (v *Variable) ExportData(fields []string) map[string]interface{} {
	e := reflect.ValueOf(v).Elem()
	fieldByName := func(val reflect.Value, field string) reflect.Value {
	return val.FieldByNameFunc(func(s string) bool {
	return strings.EqualFold(field, s)
	})
	}
	data := map[string]interface{}{}
	for _, f := range fields {
	sf := fieldByName(e, f)
	data[f] = sf.Interface()
	}
	return data
	}

[w1mvy]

It appears that secret list command does not filter by argument fields as follows

# on test repo
$ gh secret list --json name
[
  {
    "name": "HOGE",
    "updated_at": "2024-01-06T00:40:37Z",
    "visibility": "",
    "selected_repositories_url": "",
    "num_selected_repos": 0
  }
]

Therefore, I think it is necessary to implement ExportData including the secret list. How about that?

cli/api/export_pr.go

Line 8 in 586eb8b

func (issue *Issue) ExportData(fields []string) map[string]interface{} {

My implementation is inadequate and the snake case may need to be further modified as follows

func (v *Variable) ExportData(fields []string) map[string]interface{} {
	e := reflect.ValueOf(v).Elem()
	fieldByName := func(val reflect.Value, field string) reflect.Value {
		return val.FieldByNameFunc(func(s string) bool {
			return strings.EqualFold(field, s)
		})
	}
	data := map[string]interface{}{}
	for _, f := range fields {
		switch f {
		case "updated_at":
			data[f] = v.UpdatedAt
		case "selected_repositories_url":
			data[f] = v.SelectedReposURL
		case "num_selected_repos":
			data[f] = v.NumSelectedRepos
		default:
			sf := fieldByName(e, f)
			data[f] = sf.Interface()
		}
	}
	return data
}

[andyfeller]

It appears that secret list command does not filter by argument fields as follows

Oof; sounds like we have some follow up work addressing those behaviors and shortcomings, thanks for explaining 🤗 I want to bring this up with @samcoe for his thoughts as I apparently overlooked the original secret changes referenced.

[samcoe]

@w1mvy Thanks for the contribution! Since there has been fair amount of confusion internally and externally about ExportData functionality I pushed a commit to extract a helper function to ideally make this less confusing and have less boiler plate in the future. I also went ahead and fixed secret list to now take the fields requested into account. Lastly, I rebased the PR against the latest trunk. I think this is ready to 🚢 .

@andyfeller would you mind taking another quick look to verify the changes make sense to you?

[samcoe]

I would love to see all these export functions in this package moved into their own internal function named export or exportable one day, but that felt like too big of a refactor to tack onto this PR.

[samcoe]

Additionally this name is not the greatest, open to other suggestions.

[samcoe]

Having a getter and setter for the fields field felt a bit off but decided this was the quickest way to get this change in. In the future we should evaluate just exporting jsonExporter and its fields. Perhaps when doing the refactor I mentioned in the comment below.

[w1mvy]

@samcoe Thanks for your review and fix!! LGTM 👍
One small thing. With this change, the key name returned by gh secret list --json is going to be changed from snake case to camel case. (But other commands return in camel case, so I do not think this change itself is a problem. I'm thinking it's about the decision to include it in the release notes or not.)

[samcoe]

@w1mvy That is a good call out, we can definitely make sure to mention it in the release notes.

I addressed PR comments and concerns.