add verbose flag to api cmd #7826

JunNishimura · 2023-08-11T07:52:16Z

Overview

I added verbose flag to api command to output debugging information.

Explanation

Checking if GH_DEBUG=api is set is done in the NewHTTPClient function. This NewHTTPClient function is called before the NewCmdApi function which initializes api command. Therefore, it is necessary to use a slightly cumbersome approach to output debugging information in the same way as GH_DEBUG=api with the verbose flag.

My approach is to create the http client again with LogVerboseHTTP set to true when the verbose flag is set. LogVerboseHTTP is a required field to output debugging information.

There might be better approaches, so let me know if you have any ideas.

Releated Issue

Add --verbose flag to the api command #7809

andyfeller · 2023-08-14T12:52:20Z

@JunNishimura : Thank you for opening this pull request! ✨ Please allow me some time to review these changes and confirm with my team on any feedback. 🙇

JunNishimura · 2023-08-14T14:45:32Z

@andyfeller
Thanks for the message, and it is fine when you have time to spare!!

andyfeller · 2023-08-16T13:51:15Z

@JunNishimura : How extensive do you envision adding verbose logging to be?

I ask because I think there is more to adding this support than the current PR touches upon because gh api commands use different http.Client instantiation function because of unique concerns:

cli/pkg/cmd/root/root.go

Lines 154 to 159 in e4cddb5

    
           // the `api` command should not inherit any extra HTTP headers 
        
           bareHTTPCmdFactory := *f 
        
           bareHTTPCmdFactory.HttpClient = bareHTTPClient(f, version) 
        
           bareHTTPCmdFactory.BaseRepo = factory.SmartBaseRepoFunc(&bareHTTPCmdFactory) 
        
           cmd.AddCommand(apiCmd.NewCmdApi(&bareHTTPCmdFactory, nil))

cli/pkg/cmd/root/root.go

Lines 226 to 241 in e4cddb5

    
           func bareHTTPClient(f *cmdutil.Factory, version string) func() (*http.Client, error) { 
        
           	return func() (*http.Client, error) { 
        
           		cfg, err := f.Config() 
        
           		if err != nil { 
        
           			return nil, err 
        
           		} 
        
           		opts := api.HTTPClientOptions{ 
        
           			AppVersion:        version, 
        
           			Config:            cfg.Authentication(), 
        
           			Log:               f.IOStreams.ErrOut, 
        
           			LogColorize:       f.IOStreams.ColorEnabled(), 
        
           			SkipAcceptHeaders: true, 
        
           		} 
        
           		return api.NewHTTPClient(opts) 
        
           	} 
        
           }

So, we need a way that commands like gh api can the cmdutil.Factory that the http.Client needs LogVerboseHTTP set differently.

Does that make sense?

JunNishimura · 2023-08-16T23:20:58Z

@andyfeller

How extensive do you envision adding verbose logging to be?

At this stage, it is assumed that verbose logging will be added only for gh api command. However, it is possible that there will be requests to add verbose logging to other commands in the future, so it will be desirable to implement it in a versatile way.

we need a way that commands like gh api can the cmdutil.Factory that the http.Client needs LogVerboseHTTP set differently.

I assume that the implementation is to add the method to the Factory struct to set LogVerboseHTTP differently, is that correct?

andyfeller · 2023-08-17T20:29:50Z

At this stage, it is assumed that verbose logging will be added only for gh api command. However, it is possible that there will be requests to add verbose logging to other commands in the future, so it will be desirable to implement it in a versatile way.

I appreciate you having the same mindset as we have been discussing about how this work might apply to other commands in the future. 🙇

I assume that the implementation is to add the method to the Factory struct to set LogVerboseHTTP differently, is that correct?

Yeah, that is line of thought I was working through.

If api.NewHTTPClient() assumes api.HTTPClientOptions.LogVerboseHTTP is the only thing determining whether verbose logging is needed, then the func() (*http.Client, error) functions would need to check the environment for a global setting or something else for a command-specific flag.

Changing the func() (*http.Client, error) to accept an argument for LogVerboseHTTP would be problematic given everywhere it is used for generating a http.Client.

Instead, the simplest idea I can consider would be:

Add LogVerboseHTTP to cmdutil.Factory struct
Update cmd.factory.httpClientFunc() to set api.HTTPClientOptions.LogVerboseHTTP based on checking utils.IsDebugEnabled() value with f.LogVerboseHTTP
Update root.bareHTTPClient() with the same logic
Update api.NewHTTPClient() to only check opts.HTTPClientOptions.LogVerboseHTTP

Hopefully this puts us in a place where we can update other commands to signal verbose logging from a flag or globally with only the 2 places we're facilitating api.HttpClient generation.

What do you think about that?

Again, thank you for your patience working through this PR! 🫂

JunNishimura · 2023-08-19T02:49:49Z

@andyfeller
Thank you for the detailed explanation. I agree with your idea.

I've followed your ideas and implemented them, but there is one point I would like to ask.

I assign opts.LogVerboseHTTP to factory.LogVerboseHTTP in RunE (not in apiRun function) as follows. Do you see any issues with this implementation?

cli/pkg/cmd/api/api.go

Lines 221 to 223 in 5f465c9

    
           if c.Flags().Changed("verbose") { 
        
           	f.LogVerboseHTTP = opts.LogVerboseHTTP 
        
           }

samcoe · 2023-08-21T17:10:28Z

@andyfeller @JunNishimura Appreciate your work getting this feature implemented. There has been quiet a bit of back and forth on correct ways to implement this due to how connected the uses of http.Client are throughout our codebase. As it stands now I feel like this implementation has a bit too far reaching of an impact for the small feature we are trying to add. I have an alternative idea that I would like to implement, it is a bit complicated to describe though, so if no one minds I can push a commit on top of the work already done?

JunNishimura · 2023-08-21T22:54:55Z

@samcoe
Thanks for the message and contribution.

If there is a better approach, I would prefer that. So I am open to adopt your approach👍

* 'trunk' of github.com:cli/cli: Add JSON output flags to workflow list command (cli#7902) Add config clear-cache command (cli#7693) Allow --disable-wiki flag with --template flag in repo create command (cli#7886) Reword token invalid message (cli#7906) 🐛 (repo create)execute clone when readme option is enabled (cli#7837) Use go-gh prompter package (cli#7896) pr checks return distinct exit code for PENDING checks (cli#7866) Properly rename remote for GHES hosts (cli#7890) Add verbose flag to api cmd (cli#7826) Fix up Nix installation instructions (cli#7891)

JunNishimura added 5 commits August 11, 2023 16:49

add verbose flag to api cmd

efcb961

Merge branch 'trunk' into add-verbose-flag-to-api-cmd

cadef10

update Test_NewCmdApi

b029d65

update TestNewHTTPClient

c070065

add verbose test to Test_apiRun

b55b494

JunNishimura marked this pull request as ready for review August 11, 2023 09:36

JunNishimura requested a review from a team as a code owner August 11, 2023 09:36

JunNishimura requested review from vilmibm and removed request for a team August 11, 2023 09:36

cliAutomation added the external pull request originating outside of the CLI core team label Aug 11, 2023

JunNishimura changed the title ~~[WIP] add verbose flag to api cmd~~ add verbose flag to api cmd Aug 11, 2023

Merge branch 'trunk' into add-verbose-flag-to-api-cmd

b0fc59c

JunNishimura added 3 commits August 19, 2023 01:36

add LobVerboseHTTP to factory

73d1f7a

fix http_client_test

2a886be

Merge branch 'trunk' into add-verbose-flag-to-api-cmd

5f465c9

fix opts.LogVerboseHTTP set logic

14a7ccf

Simplify implementation

9052eb3

samcoe self-assigned this Aug 25, 2023

samcoe approved these changes Aug 25, 2023

View reviewed changes

samcoe merged commit 508065b into cli:trunk Aug 25, 2023

JunNishimura deleted the add-verbose-flag-to-api-cmd branch August 25, 2023 17:03

andyfeller mentioned this pull request Oct 2, 2023

Add global --debug flag #5954

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

add verbose flag to api cmd #7826

add verbose flag to api cmd #7826

Uh oh!

JunNishimura commented Aug 11, 2023 •

edited

Loading

Uh oh!

andyfeller commented Aug 14, 2023

Uh oh!

JunNishimura commented Aug 14, 2023

Uh oh!

andyfeller commented Aug 16, 2023 •

edited

Loading

Uh oh!

JunNishimura commented Aug 16, 2023

Uh oh!

andyfeller commented Aug 17, 2023

Uh oh!

JunNishimura commented Aug 19, 2023

Uh oh!

samcoe commented Aug 21, 2023

Uh oh!

JunNishimura commented Aug 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

add verbose flag to api cmd #7826

add verbose flag to api cmd #7826

Uh oh!

Conversation

JunNishimura commented Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Explanation

Releated Issue

Uh oh!

andyfeller commented Aug 14, 2023

Uh oh!

JunNishimura commented Aug 14, 2023

Uh oh!

andyfeller commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JunNishimura commented Aug 16, 2023

Uh oh!

andyfeller commented Aug 17, 2023

Uh oh!

JunNishimura commented Aug 19, 2023

Uh oh!

samcoe commented Aug 21, 2023

Uh oh!

JunNishimura commented Aug 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JunNishimura commented Aug 11, 2023 •

edited

Loading

andyfeller commented Aug 16, 2023 •

edited

Loading