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

fix: support import theme from git repo #271

Merged
merged 1 commit into from
Jul 13, 2023
Merged

fix: support import theme from git repo #271

merged 1 commit into from
Jul 13, 2023

Conversation

golangboy
Copy link
Contributor

支持从git仓库clone主题

service/impl/theme.go Fixed Show fixed Hide fixed
@golangboy
Copy link
Contributor Author

@1379

service/impl/theme.go Outdated Show resolved Hide resolved
@1379
Copy link
Contributor

1379 commented Jul 12, 2023

@golangboy Well done!

  • 有几个lint 错误,可以修复下。
  • go-git 的实现已经从src-d/go-git 迁移到了 go-git/go-git

@1379
Copy link
Contributor

1379 commented Jul 12, 2023

主题的下载需要适配多种实现,可以看下MultipartZipThemeFetcher 这个

@golangboy
Copy link
Contributor Author

@1379 我发现这个函数的主题是下载到系统的临时目录的

func (t *themeServiceImpl) UploadTheme(ctx context.Context, file *multipart.FileHeader) (*dto.ThemeProperty, error) {
	themeProperty, err := t.MultipartZipThemeFetcher.FetchTheme(ctx, file)
	if err != nil {
		return nil, err
	}
	return t.addTheme(ctx, themeProperty)
}
func (m *multipartZipThemeFetcherImpl) FetchTheme(ctx context.Context, file interface{}) (*dto.ThemeProperty, error) {
	themeFileHeader, ok := file.(*multipart.FileHeader)
	if !ok {
		return nil, xerr.WithStatus(nil, xerr.StatusBadRequest).WithMsg("not support")
	}

	tempDir := os.TempDir()
       ...

临时目录是临时存放文件目录的,随时可能被清除,其实可以在上层获取到主题的存放目录

func (t *themeServiceImpl) UploadTheme(ctx context.Context, file *multipart.FileHeader) (*dto.ThemeProperty, error) {
	// 这里可以通过t.Config.Sonic.ThemeDir获取主题目录
	themeProperty, err := t.MultipartZipThemeFetcher.FetchTheme(ctx, file)
	if err != nil {
		return nil, err
	}
	return t.addTheme(ctx, themeProperty)
}

@1379
Copy link
Contributor

1379 commented Jul 12, 2023

@golangboy themeFetcher 只负责下载主题,并不关心主题的存放位置。addTheme方法会把临时目录的主题Copy到主题目录下的

@1379
Copy link
Contributor

1379 commented Jul 12, 2023

用策略模式去实现更优吧?

@golangboy
Copy link
Contributor Author

@1379

@1379
Copy link
Contributor

1379 commented Jul 12, 2023

  • 还是有一些lint错误(你可以在'File Changes' 中看到)
  • 能否换成策略模式去实现呢,类似service/storage/storage.go,以及已有的MultipartZipThemeFetcher

@golangboy
Copy link
Contributor Author

  • 还是有一些lint错误(你可以在'File Changes' 中看到)
  • 能否换成策略模式去实现呢,类似service/storage/storage.go,以及已有的MultipartZipThemeFetcher

我看到其中有一个lint error是这样的

[lint: service/impl/theme.go#L12](https://github.com/go-sonic/sonic/pull/271/files#annotation_12554936293)
import 'github.com/go-git/go-git/v5' is not allowed from list 'Main' (depguard)

我查了一下 depguard ,发现是依赖一个文件.depguard.yml 但是项目里似乎是没有的?

@1379
Copy link
Contributor

1379 commented Jul 12, 2023

@golangboy 那就先忽略掉这个错误吧。
另外go.mod 不太符合规范哦,需要执行go mod tidy

@golangboy
Copy link
Contributor Author

@1379

@1379
Copy link
Contributor

1379 commented Jul 12, 2023

可能是我之前的命名误导你了,抱歉 :)

type MultipartZipThemeFetcher interface {
	FetchTheme(ctx context.Context, file interface{}) (*dto.ThemeProperty, error)
}

应该改成

type ThemeFetcher interface {
	FetchTheme(ctx context.Context, file interface{}) (*dto.ThemeProperty, error)
}

对应的,新的GitThemeFetcher也应该去实现这个接口,你觉得如何。

@golangboy
Copy link
Contributor Author

@1379


diskFilePath := filepath.Join(tempDir, fileName)
if util.FileIsExisted(diskFilePath) {
err = os.Remove(diskFilePath)

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression

This path depends on a [user-provided value](1).
}
}

diskFile, err := os.OpenFile(diskFilePath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o444)

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression

This path depends on a [user-provided value](1).
if err == nil && themeProperty != nil {
return themeProperty, nil
}
dirEntrys, err := os.ReadDir(dest)

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression

This path depends on a [user-provided value](1).
@1379
Copy link
Contributor

1379 commented Jul 13, 2023

Bravo!
还有几个lint错误可以修复。

@golangboy
Copy link
Contributor Author

@1379

@1379
Copy link
Contributor

1379 commented Jul 13, 2023

Awesome!

Copy link
Contributor

@1379 1379 left a comment

Choose a reason for hiding this comment

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

Thanks

@1379
Copy link
Contributor

1379 commented Jul 13, 2023

use rebase instead of merge

@golangboy
Copy link
Contributor Author

golangboy commented Jul 13, 2023

use rebase instead of merge

没理解

@1379
Copy link
Contributor

1379 commented Jul 13, 2023

这里是指git 操作。我来操作也可以的

@1379 1379 merged commit c2450ab into go-sonic:master Jul 13, 2023
@1379
Copy link
Contributor

1379 commented Jul 13, 2023

Done!

@golangboy golangboy deleted the fetchtheme branch July 13, 2023 13:07
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.

2 participants