Repository: cli/cli
File: pkg/cmd/repo/edit/edit.go (Go)

Code before:
	if hasSecurityEdits(opts.Edits) {
		apiClient := api.NewClientFromHTTP(opts.HTTPClient)
		repo, err := api.FetchRepository(apiClient, opts.Repository, []string{"viewerCanAdminister"})
		if err != nil {
			return err
		}
		if !repo.ViewerCanAdminister {
			return fmt.Errorf("you do not have sufficient permissions to edit repository security and analysis features")
		}

		opts.Edits.SecurityAndAnalysis = &SecurityAndAnalysisInput{}
		if opts.Edits.enableAdvancedSecurity != nil {
			opts.Edits.SecurityAndAnalysis.EnableAdvancedSecurity = &SecurityAndAnalysisStatus{
				Status: boolToStatus(*opts.Edits.enableAdvancedSecurity),
			}
		}
		if opts.Edits.enableSecretScanning != nil {
			opts.Edits.SecurityAndAnalysis.EnableSecretScanning = &SecurityAndAnalysisStatus{
				Status: boolToStatus(*opts.Edits.enableSecretScanning),
			}
		}
		if opts.Edits.enableSecretScanningPushProtection != nil {
			opts.Edits.SecurityAndAnalysis.EnableSecretScanningPushProtection = &SecurityAndAnalysisStatus{
				Status: boolToStatus(*opts.Edits.enableSecretScanningPushProtection),
			}
		}
	}

	apiPath := fmt.Sprintf("repos/%s/%s", repo.RepoOwner(), repo.RepoName())

	body := &bytes.Buffer{}
	enc := json.NewEncoder(body)
	if err := enc.Encode(opts.Edits); err != nil {
		return err
	}

	g := errgroup.Group{}

	if body.Len() > 3 {
		g.Go(func() error {
			apiClient := api.NewClientFromHTTP(opts.HTTPClient)
			_, err := api.CreateRepoTransformToV4(apiClient, repo.RepoHost(), "PATCH", apiPath, body)
			return err
		})
	}

	if len(opts.AddTopics) > 0 || len(opts.RemoveTopics) > 0 {
		g.Go(func() error {
			// opts.topicsCache gets populated in interactive mode
			if !opts.InteractiveMode {
				var err error

Review comment (security):
I think I'd like to see these moved into their own helper function and to be moved up into the `RunE` of `NewCmdEdit`. That way, we can unit test the transformer (i.e. the thing that turns the flag booleans into the struct format) and inject the `SecurityAndAnalysis` struct on `opts` as a dependency into `editRun` for testing purposes.

I was really confused at the start of this review as to why this looked so different from the other repo properties. I finally found my answer in the [API docs](https://docs.github.com/en/rest/repos/repos?apiVersion=2022-11-28#update-a-repository). I think this change will help future contributors grok this faster than I did, too.

Let me know if you have questions. I'm also happy to add a commit or PR into this branch to show you what I mean 🙂 

Code after:
		if !repo.ViewerCanAdminister {
			return fmt.Errorf("you do not have sufficient permissions to edit repository security and analysis features")
		}
	}

	apiPath := fmt.Sprintf("repos/%s/%s", repo.RepoOwner(), repo.RepoName())

	body := &bytes.Buffer{}
	enc := json.NewEncoder(body)
	if err := enc.Encode(opts.Edits); err != nil {
		return err
	}

	g := errgroup.Group{}

	if body.Len() > 3 {
		g.Go(func() error {
			apiClient := api.NewClientFromHTTP(opts.HTTPClient)
			_, err := api.CreateRepoTransformToV4(apiClient, repo.RepoHost(), "PATCH", apiPath, body)
			return err
		})
	}

	if len(opts.AddTopics) > 0 || len(opts.RemoveTopics) > 0 {
		g.Go(func() error {
			// opts.topicsCache gets populated in interactive mode
			if !opts.InteractiveMode {
				var err error
				opts.topicsCache, err = getTopics(ctx, opts.HTTPClient, repo)
				if err != nil {
					return err
				}
			}
			oldTopics := set.NewStringSet()
			oldTopics.AddValues(opts.topicsCache)

			newTopics := set.NewStringSet()
			newTopics.AddValues(opts.topicsCache)
			newTopics.AddValues(opts.AddTopics)
			newTopics.RemoveValues(opts.RemoveTopics)

			if oldTopics.Equal(newTopics) {
				return nil
			}
			return setTopics(ctx, opts.HTTPClient, repo, newTopics.ToSlice())
		})
	}

	err := g.Wait()
	if err != nil {
		return err