Skip to content

When issue template name ends with .yaml/.yml call the yamlParser #23719

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 6 additions & 10 deletions modules/label/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,21 +36,17 @@ func (err ErrTemplateLoad) Error() string {
// GetTemplateFile loads the label template file by given name,
// then parses and returns a list of name-color pairs and optionally description.
func GetTemplateFile(name string) ([]*Label, error) {
data, err := options.Labels(name + ".yaml")
if err == nil && len(data) > 0 {
return parseYamlFormat(name+".yaml", data)
// Always check if <name>.yaml or <name>.yml exists and prefer those
data, extension, err := options.Labels(name, ".yaml", ".yml", "")
if err != nil {
return nil, ErrTemplateLoad{name, fmt.Errorf("GetRepoInitFile: %w", err)}
}

data, err = options.Labels(name + ".yml")
if err == nil && len(data) > 0 {
// because we only handle .yaml/.yml we can simply test if the extension is not empty
if len(extension) > 0 && len(data) > 0 {
return parseYamlFormat(name+".yml", data)
}

data, err = options.Labels(name)
if err != nil {
return nil, ErrTemplateLoad{name, fmt.Errorf("GetRepoInitFile: %w", err)}
}

return parseLegacyFormat(name, data)
}

Expand Down
21 changes: 19 additions & 2 deletions modules/options/base.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ func License(name string) ([]byte, error) {
}

// Labels reads the content of a specific labels from static/bindata or custom path.
func Labels(name string) ([]byte, error) {
return fileFromOptionsDir("label", name)
func Labels(name string, exts ...string) ([]byte, string, error) {
return fileFromOptionsDirExtensions([]string{"label", name}, exts...)
}

// WalkLocales reads the content of a specific locale
Expand Down Expand Up @@ -132,3 +132,20 @@ func readLocalFile(baseDirs []string, subDir string, elems ...string) ([]byte, e
}
return nil, os.ErrNotExist
}

func readLocalFileExtensions(baseDirs []string, subDir string, elems []string, extensions ...string) ([]byte, string, error) {
if len(extensions) == 0 {
extensions = append(extensions, "")
}
for _, localPath := range joinLocalPaths(baseDirs, subDir, elems...) {
for _, extension := range extensions {
data, err := os.ReadFile(localPath + extension)
if err == nil {
return data, extension, nil
} else if !os.IsNotExist(err) {
log.Error("Unable to read file %q. Error: %v", localPath, err)
}
}
}
return nil, "", os.ErrNotExist
}
5 changes: 5 additions & 0 deletions modules/options/dynamic.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ func fileFromOptionsDir(elems ...string) ([]byte, error) {
return readLocalFile([]string{setting.CustomPath, setting.StaticRootPath}, "options", elems...)
}

// fileFromOptionsDirExtensions is a helper to read files from custom or static path.
func fileFromOptionsDirExtensions(elems []string, extensions ...string) ([]byte, string, error) {
return readLocalFileExtensions([]string{setting.CustomPath, setting.StaticRootPath}, "options", elems, extensions...)
}

// IsDynamic will return false when using embedded data (-tags bindata)
func IsDynamic() bool {
return true
Expand Down
30 changes: 30 additions & 0 deletions modules/options/static.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ package options
import (
"fmt"
"io"
"os"

"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"
Expand Down Expand Up @@ -66,6 +67,35 @@ func fileFromOptionsDir(elems ...string) ([]byte, error) {
return io.ReadAll(f)
}

// fileFromOptionsDir is a helper to read files from custom path or bindata.
func fileFromOptionsDirExtensions(elems []string, extensions ...string) ([]byte, string, error) {
// only try custom dir, no static dir
if data, extension, err := readLocalFileExtensions([]string{setting.CustomPath}, "options", elems, extensions...); err == nil {
return data, extension, nil
}

if len(extensions) == 0 {
extensions = append(extensions, "")
}

for _, extension := range extensions {
f, err := Assets.Open(util.PathJoinRelX(elems...))
if err != nil {
if os.IsNotExist(err) {
continue
}
return nil, "", err
}
defer f.Close()
bs, err := io.ReadAll(f)
if err != nil {
return nil, "", err
}
return bs, extension, nil
}
return nil, "", os.ErrNotExist
}

func Asset(name string) ([]byte, error) {
f, err := Assets.Open("/" + name)
if err != nil {
Expand Down
36 changes: 20 additions & 16 deletions modules/repository/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,18 +45,26 @@ var (
func LoadRepoConfig() {
// Load .gitignore and license files and readme templates.
types := []string{"gitignore", "license", "readme", "label"}
typeFiles := make([][]string, 4)

removeExtension := func(f string) string {
ext := strings.ToLower(filepath.Ext(f))
if ext == ".yaml" || ext == ".yml" {
return f[:len(f)-len(ext)]
}
return f
}
Comment on lines +49 to +55
Copy link
Contributor

@wxiaoguang wxiaoguang Mar 27, 2023

Choose a reason for hiding this comment

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

I do not understand why it should remove the extensions. It looks hacky and forces other code to "guess" the file name again and again.

And could there be a case like this:

  • Gitea has builtin Advanced.yaml
  • A user puts a customized Advance.yml
  • Then still the builtin Advanced.yaml is used when guessing the name "Advanced"?

IIRC the options package tries the custom directory first and then the builtin assets.

  • If there are two files: custom/Advance.yml and static/Advance.yaml
  • If load("Advance") tries Advance.yaml first, it would always load static/Advance.yaml ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's completely reasonable to hide the extension - however, this does make overriding a little bit more complex.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or could the extension just be hidden on the UI? But always use the filename with extension internally?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or could the extension just be hidden on the UI? But always use the filename with extension internally?

How do you think about this proposal? I think it has more Pros and less Cons than current PR's approach.

We have already been doing the best to avoid guess "refs" for branch/tag/commits. Now, I don't think it makes sense to guess the extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you can't guarantee that the templates file hasn't been changed on the filesystem. So - therefore you cannot know the extension at the time of looking up the file.

Therefore you need to guess.

Copy link
Contributor

@wxiaoguang wxiaoguang Mar 27, 2023

Choose a reason for hiding this comment

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

My proposal is Respect the origin file names of option items #23749

I think we do not need to guess. If you think there is any problem, please help to design a case for it (to help me to understand the problem).


labelTemplatesFiles := []string{}
typeFiles := []*[]string{&Gitignores, &Licenses, &Readmes, &labelTemplatesFiles}

for i, t := range types {
files, err := options.Dir(t)
if err != nil {
log.Fatal("Failed to get %s files: %v", t, err)
}
if t == "label" {
for i, f := range files {
ext := strings.ToLower(filepath.Ext(f))
if ext == ".yaml" || ext == ".yml" {
files[i] = f[:len(f)-len(ext)]
}
files[i] = removeExtension(f)
}
}
customPath := path.Join(setting.CustomPath, "options", t)
Expand All @@ -71,26 +79,22 @@ func LoadRepoConfig() {
}

for _, f := range customFiles {
if t == "label" {
f = removeExtension(f)
}

if !util.SliceContainsString(files, f, true) {
files = append(files, f)
}
}
}
typeFiles[i] = files
sort.Strings(files)
*typeFiles[i] = files
}

Gitignores = typeFiles[0]
Licenses = typeFiles[1]
Readmes = typeFiles[2]
LabelTemplatesFiles := typeFiles[3]
sort.Strings(Gitignores)
sort.Strings(Licenses)
sort.Strings(Readmes)
sort.Strings(LabelTemplatesFiles)

// Load label templates
LabelTemplates = make(map[string]string)
for _, templateFile := range LabelTemplatesFiles {
for _, templateFile := range labelTemplatesFiles {
labels, err := label.LoadFormatted(templateFile)
if err != nil {
log.Error("Failed to load labels: %v", err)
Expand Down