Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor repo clone logic to support multi platform and authentication #460

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CarlJi
Copy link
Contributor

@CarlJi CarlJi commented Nov 22, 2024

Checkpoint:

Supports run with

  • Only GitLab
  • Only GitHub
  • Both

For GitHub

  • Can be downloaded and used via the app.
  • Can be downloaded and used via Access Token.
  • Can be downloaded and used via OAuth.

GitLab

  • Can be downloaded and used via Access Token.
  • Can be downloaded and used via OAuth Token.

For Runner

  • Able to pass token

Copy link

netlify bot commented Nov 22, 2024

Deploy Preview for reviewbot-x canceled.

Name Link
🔨 Latest commit cef0726
🔍 Latest deploy log https://app.netlify.com/sites/reviewbot-x/deploys/67402f7dcee324000866e0dd

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 0.61350% with 324 lines in your changes missing coverage. Please review.

Project coverage is 25.06%. Comparing base (6af266d) to head (cef0726).

Files with missing lines Patch % Lines
clone.go 0.00% 295 Missing ⚠️
main.go 0.00% 15 Missing ⚠️
server.go 0.00% 13 Missing ⚠️
internal/linters/providergitlab.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #460      +/-   ##
==========================================
- Coverage   26.36%   25.06%   -1.30%     
==========================================
  Files          31       32       +1     
  Lines        4358     4572     +214     
==========================================
- Hits         1149     1146       -3     
- Misses       3071     3288     +217     
  Partials      138      138              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@CarlJi CarlJi force-pushed the feat/token branch 4 times, most recently from 3133720 to a31b6d5 Compare November 22, 2024 07:12
gitv2 "sigs.k8s.io/prow/pkg/git/v2"
)

func (s *Server) prepareGitRepos(ctx context.Context, org, repo string, num int, platform config.Platform, installationID int64) (workspace string, workDir string, err error) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[golangci-lint] reported by reviewbot🐮
cognitive complexity 33 of func (*Server).prepareGitRepos is high (> 30) (gocognit)

case auth.GitHubAppAuth != nil:
opt.UseSSH = github.Bool(false)
opt.Username = func() (string, error) {
return "x-access-token", nil
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[golangci-lint] reported by reviewbot🐮
string x-access-token has 3 occurrences, make it a constant (goconst)

return token, nil
}

// refreshToken refresh the GitHub App token
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[golangci-lint] reported by reviewbot🐮
Comment should end in a period (godot)

}

// main repo, need to checkout PR and update submodules if any
if ref.Org == org && ref.Repo == repo {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[golangci-lint] reported by reviewbot🐮
if ref.Org == org && ref.Repo == repo has complex nested blocks (complexity: 5) (nestif)


gitConfig := s.newGitConfigBuilder(ref.Org, ref.Repo, platform, installationID).Build()
log.Debugf("git config: %+v", gitConfig)
if err := s.configureGitAuth(&opt, gitConfig); err != nil {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[golangci-lint] reported by reviewbot🐮
Function configureGitAuth->configureGitHubAuth->configureGitHubAuth$2->getToken->refreshToken should pass the context parameter (contextcheck)

case config.GitLab:
return s.configureGitLabAuth(opt, gConf)
default:
return fmt.Errorf("unsupported platform: %s", gConf.Platform)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[golangci-lint] reported by reviewbot🐮
do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("unsupported platform: %s", gConf.Platform)" (err113)

Details 这个警告提醒你不要定义**动态错误**,而是应该使用**静态错误**并通过错误包装来处理错误。

背景

在 Go 语言中,我们可以通过 fmt.Errorf 动态创建错误,尤其是带有格式化字符串的错误信息。然而,对于一些固定的错误信息,不建议每次都动态生成新的错误对象。相反,应该使用预定义的、可复用的静态错误(通常是一个包级别的 var 错误变量),并在需要时对错误进行包装或附加上下文。

原因

  1. 复用和比较:在 Go 中,通过 errors.Is 或直接比较错误值,可以轻松判断错误的类型。如果每次都通过 fmt.Errorf 动态生成错误对象,那么这些错误对象是不同的,无法直接进行比较。

  2. 性能:静态错误的内存分配只会发生一次,而动态生成的错误会在每次生成时分配新的内存,增加不必要的开销。

  3. 可读性:使用静态错误可以让代码更加简洁和可维护,因为常见的错误可以在包级别定义并复用。

示例

错误的做法(动态错误)

// 动态生成错误信息
err := fmt.Errorf("docker image is not set")

这种方式会每次生成一个新的错误对象,导致错误无法复用和比较。

正确的做法(使用静态错误)

你可以定义一个静态的错误变量,然后在需要时用它来表示特定类型的错误:

// 定义一个静态错误
var ErrDockerImageNotSet = errors.New("docker image is not set")

// 使用静态错误
if someCondition {
    return ErrDockerImageNotSet
}

这样做有几个好处:

  1. 复用ErrDockerImageNotSet 是静态定义的,可以在不同的地方复用。

  2. 比较:可以通过 errors.Is 或直接比较来判断错误类型:

    if errors.Is(err, ErrDockerImageNotSet) {
        // 处理 docker image 未设置的错误
    }

包装错误以增加上下文信息

如果你想使用静态错误并且还想在错误中附加一些上下文信息,可以使用 fmt.Errorf"%w" 语法来包装静态错误,而不是动态生成新的错误:

var ErrDockerImageNotSet = errors.New("docker image is not set")

// 包装错误并添加上下文信息
return fmt.Errorf("failed to start container: %w", ErrDockerImageNotSet)

在这个例子中,我们保留了静态错误 ErrDockerImageNotSet,并通过 %w 包装错误,保持了错误可比较的特性,同时附加了上下文信息 "failed to start container"

总结

  • fmt.Errorf("docker image is not set") 是一种动态错误的创建方式,每次都会生成新的错误对象。
  • 静态错误是通过 errors.Newvar 定义的全局错误变量,便于复用和比较。
  • 在需要提供上下文信息时,可以用 fmt.Errorf"%w" 包装静态错误,而不是直接创建新的动态错误。

因此,err113 的意思是:不要使用动态生成的错误,而应定义静态错误并在需要时进行包装

}

func (b *GitConfigBuilder) getHostForPlatform(platform config.Platform) string {
switch platform {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[golangci-lint] reported by reviewbot🐮
missing cases in switch of type config.Platform: config.GitHub (exhaustive)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant