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

zeripath · 2023-03-26T17:47:11Z

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

zeripath · 2023-03-26T17:47:33Z

I may have the wrong end of the stick here - but this seems more like the correct solution.

wxiaoguang · 2023-03-26T17:53:02Z

I think the root problem is here: https://github.com/go-gitea/gitea/pull/22976/files#diff-39f732f012278472d9dc6f19b2deb4c63e1283f649a33203ed4ced67e804f8a2R54-R61

It shouldn't remove the ext-name of these files. Without the correct ext-name, other code has to "guess" the file name again and again, then various bugs would come.

I think it's better to keep the ext-names, and only remove the ext-name when rendering the dropdown UI (or, just keep the ext-names on UI)

codecov-commenter · 2023-03-26T18:14:52Z

Codecov Report

Merging #23719 (5fef66a) into main (f521e88) will decrease coverage by 0.02%.
The diff coverage is 41.05%.

❗ Current head 5fef66a differs from pull request most recent head e43bd49. Consider uploading reports for the commit e43bd49 to get more accurate results

@@            Coverage Diff             @@
##             main   #23719      +/-   ##
==========================================
- Coverage   47.14%   47.12%   -0.02%     
==========================================
  Files        1149     1155       +6     
  Lines      151446   152401     +955     
==========================================
+ Hits        71397    71817     +420     
- Misses      71611    72108     +497     
- Partials     8438     8476      +38

Impacted Files	Coverage Δ
cmd/dump.go	`0.66% <0.00%> (-0.01%)`	⬇️
cmd/web.go	`0.00% <0.00%> (ø)`
models/actions/run.go	`1.64% <0.00%> (-0.08%)`	⬇️
models/actions/runner.go	`1.44% <ø> (ø)`
models/packages/package.go	`45.45% <0.00%> (-1.13%)`	⬇️
models/user/search.go	`77.50% <0.00%> (-6.29%)`	⬇️
modules/actions/workflows.go	`0.00% <0.00%> (ø)`
modules/context/context.go	`64.54% <0.00%> (-3.53%)`	⬇️
modules/doctor/storage.go	`30.65% <0.00%> (-1.29%)`	⬇️
modules/label/parser.go	`55.26% <0.00%> (-3.90%)`	⬇️
... and 38 more

... and 45 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

eeyrjmr · 2023-03-26T18:33:05Z

I think the root problem is here: https://github.com/go-gitea/gitea/pull/22976/files#diff-39f732f012278472d9dc6f19b2deb4c63e1283f649a33203ed4ced67e804f8a2R54-R61

It shouldn't remove the ext-name of these files. Without the correct ext-name, other code has to "guess" the file name again and again, then various bugs would come.

I think it's better to keep the ext-names, and only remove the ext-name when rendering the dropdown UI (or, just keep the ext-names on UI)

This is probably 1/2 the issue. This removes the extensions from the files in the label directory. Why does it have to be stripped? just acknowledge that you could have

foo
foo.yaml
foo.yml

True this possibly will cause other issues but that's probably what is going on here.

I may have the wrong end of the stick here - but this seems more like the correct solution.
Possibly...
100% what you have written is a lot cleaner than what I wrote in #23717 ( wanted an || guard on the extension but I am REALLY new to golang).

The bigger question though is it isn't solving the issue.

Original code:

GetTemplateFile(...)
* Is it *.yaml?  -> execute parseYamlFormat
* Is it *.yml?  -> execute parseYamlFormat
* execute parseLegacyFormat

Thus a file with a yaml (or yml) is parsed twice and thus the WebUI will show one valid option and one invalid as one of the parsers could fail. Likewise it could be a yaml file but legacy format

My proposal

GetTemplateFile(...)
1. Is it *.yaml?  -> execute parseYamlFormat
OR 
2. Is it *.yml?  -> execute parseYamlFormat
OR
3. execute parseLegacyFormat

An attempt was made to only parse once for a file in the list.

Your proposal

GetTemplateFile(...)
* Is it *.yaml || *.yml ?  -> execute parseYamlFormat
* Is it *.yaml?  -> execute parseYamlFormat
* execute parseLegacyFormat

A yaml file will be parsed three times and have the WebUI issue. A yml file will be parsed twice and have the WebUI issue

The solution will be merging aspects of the three MR as each attack the issue from where the issue is found.

There is then the decision what todo if three files exist: {foo,foo.yml,foo.yaml} should the yaml take priority of the shorted extension and should it override the legacy fileformat of the same basename?

wxiaoguang · 2023-03-26T19:11:42Z

Why does it have to be stripped?

TBH, I don't know, while I don't see a strong reason to strip the extensions. Why not just keep them and show all of them to end users? Since these files are configured by the site admin, end users won't be surprised.

This is probably 1/2 the issue. This removes the extensions from the files in the label directory. Why does it have to be stripped? just acknowledge that you could have
1. foo
2. foo.yaml
3. foo.yml
True this possibly will cause other issues but that's probably what is going on here.

In my mind, if there is no clearly defined behavior, bugs can't be completely eliminated.

If we can define the behavior clearly (eg: like above, make every name strictly clear), then I guess there won't be such bug?

zeripath · 2023-03-27T12:12:58Z

OK I understand this a bit better.

I think GetTemplateFile was always correct - the issue is in modules/repository/init.go.

I'll push up a change.

…o options The previous code did not remove the .yaml/.yml extension from label files within the customPath. This would lead to duplicate lists of labels. Fix go-gitea#23715 Close go-gitea#23717 Signed-off-by: Andrew Thornton <art27@cantab.net>

wxiaoguang · 2023-03-27T13:45:06Z

modules/repository/init.go

+	removeExtension := func(f string) string {
+		ext := strings.ToLower(filepath.Ext(f))
+		if ext == ".yaml" || ext == ".yml" {
+			return f[:len(f)-len(ext)]
+		}
+		return f
+	}


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 ?

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

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

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.

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.

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).

Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath · 2023-03-28T06:40:07Z

#23749 will break functionality allowing users to drop-in replacements for labels whilst gitea is running

wxiaoguang · 2023-03-28T06:54:21Z

#23749 will break functionality allowing users to drop-in replacements for labels whilst gitea is running

But this PR doesn't fix it either. Please show a real case. I can show more cases based on your cases for this PR too.

wxiaoguang · 2023-03-28T06:58:26Z

Let's imagine a case:

A site admin runs Gitea
They want to put "MyCoolLabel.yaml" into the custom template
Neither your PR nor my PR would make the Gitea find the newly added files.
Then the site admin should restart the Gitea server

For your case:

A site admin runs Gitea, it already has builtin "Advance.yaml".
They put a "Advance.yml" into the custom template directory.
My PR: do not load the newly added "Advance.yml", just like above, the consistent behavior.
Your PR: could load the "Advance.yml" , it is somewhat better, but not a must.

And even more, if your PR sees "Advance.yaml" and "Adanvce.yml" at the same time in custom template directory, which one would it use? Would it make users feel surprised that only one is used?

My PR just loads both of them, I would say my PR's behavior is better: more stable, more consistent, no surprise.

eeyrjmr · 2023-03-28T08:18:09Z

There is a 3rd option... Warn.

So the problem statement is the label parser can be triggered "at least" twice if a yaml file is present resulting in an invalid WebUI when a yaml file is parsed by the Legacy Parser

options/label/test -> call parseLegacyFormat
options/label/test.yaml -> call parseYamlFormat and parseLegacyFormat
options/label/test.yml -> call parseYamlFormat and parseLegacyFormat

What happens when all three are present? right now only the legacy format is usable BUT the two appear in the WebUI, with their extensions... while they are parsed the "data" is thrownaway.

Lets assume the parsing actually worked and all three were parsed, selectable and usable... Should they be?
From an admin perspective this was a silly mistake and from a user perspective "what should I use" and thus a non-deterministic user response if they want to use label option test, more of a concern if there is subtle differences between the three (colours, exclusivity etc...)

Option 1

Parse all 3 and display all 3

Pro: Permissively accept a clear admin error
Con: User experience isn't the greatest and a user is likely to complain and an admin removes redundant files

Option 2

permit only extension yaml when presented with test or test.yaml.

Pro: Simpler, aligns with the advice to prefer the advance format as stated here: https://docs.gitea.io/en-us/customizing-gitea/#labels
Con: silently drop yml files if an admin didn't use the yaml extension

Option 3

Warn that a shadow file exists via some WebUI banner (like UTF-8 stuff)

Pro: clearly indicates there is an issue
Con: a lot more coding

Its not unusual for a toolsuite to warn about shadow files and not use them ( example of Matlab: https://uk.mathworks.com/help/simulink/ug/manage-shadowed-and-dirty-model-files.html ) but as stated, that would appear a lot of work...

Personally Option2 would be the simplest and cleanest.

wxiaoguang · 2023-03-28T08:37:29Z

Either Option 1 or Option 2 could be implemented in my proposal too.

At the moment , I think Option 1 is the best (the current #23749). Because :

Option 1: Con: User experience isn't the greatest and a user is likely to complain and an admin removes redundant files

That's really site admin's duty to make sure the files in custom directory are all correct.

And now site admin could "hide" builtin label files by putting an empty file in the custom directory, which is better. Before, I guess it was impossible.

Option 2 permit only extension yaml when presented with test or test.yaml.

It has bugs. For example, you have builtin Advance.yaml and put a custom Advance, by this logic, the custom one is never used.

I do not think Option 2 is a complete solution.

Option 3 Warn that a shadow file exists via some WebUI banner (like UTF-8 stuff)

I do not think it needs to be that complex, because it's only a site admin work, and usually it only needs to be done once (or only a few times). End users do not need to know any detail of it.

Actually I think my proposal #23749 is simple and clear enough, and I can not see a bad case for it (if there is one, feel free to provide more details and I'd like to learn more).

zeripath · 2023-03-29T12:49:48Z

@wxiaoguang the problem with passing the filename to the UI is that it is fundamentally the wrong thing.

Users are NOT choosing a file for the labels - they're choosing a label-set.

wxiaoguang · 2023-03-29T12:54:25Z

I have strong objection about this approach.

MyLabel.yaml and MyLabel.yml are different label sets, they should never be mixed together.

This PR makes things unnecessarily complicated, make code difficult to maintain, brings no real benefit to end users.

eeyrjmr · 2023-03-29T13:06:57Z

I agree with @zeripath in as far as the choice is a label set and those that choose a label set are owners of repositories or org... Does such users care it's legacy,yml or yaml as long as the choice they need exists.

Thing is ... I agree with @wxiaoguang if different files exist they might be different labels .

This comes back to the comment in the other PR, this is a site admin mistake as the existance of such files is just systematically wrong.
Let's say I write a procedure that org owners shall pick label set MyLabel but as site admin I place three files (MyLabel, MyLabel.yml, MyLabel.yaml ), what are the owners todo?

How much accomodation for bad file customisation should gitea accommodate ?
gitea uses golang templates alot. What if a site admin started using *.gotmpl instead of *.tmpl ?
gotmpl is an accepted alternative extension and I suspect gitea would ignore any such extention. If so there is precidence on supporting a single extension: yaml (lowercase )

This simplifies the discussion to legacyparser Vs yamlparser and thus "show both" with a plan to depreciate legacy in say.... v1.100

wxiaoguang · 2023-03-29T13:13:04Z

How much accomodation for bad file customisation should gitea accommodate ?

IMO Gitea shouldn't spend time on accommodating bad file, so I prefer to just show them as they are, let the site admin handle them.

In history, there were too many dirty patches in Gitea's code base, many patches becomes technical debt today and causes new bugs.

I would say "guessing" option file extensions is also a kind of technical debt, it makes the logic and behavior unclear, and it's difficult to maintain or test.

jolheiser · 2023-03-29T14:50:09Z

One case I like is this allows an admin to override Default with their own Default.yaml

Does yours allow for that @wxiaoguang?

wxiaoguang · 2023-03-29T14:51:53Z

Does yours allow for that @wxiaoguang?

Yes, put an empty Default file there.

wxiaoguang · 2023-03-29T14:53:51Z

And I can have another approach without putting an empty file:

If Xxxx.yaml exists, then remove the Xxxx from the list, to make it "overwrite", that's still a clear behavior without guessing extensions.

I didn't do that because I already impelmented the empty file approach before, it's more flexible, could just "hide" anything they do not like, eg: Advanced.yaml. Otherwise, this PR couldn't hide the unwanted label-sets.

Update: done in 18a5258 , since many people like it.

zeripath · 2023-03-29T16:23:21Z

Fundamentally the problem with #23749 is that it is the case that it doesn't matter what is being presented to the user as the name - the filename with/or without the extension is being presented as the internal API - when it fundamentally was not the API.

labels.yaml overrules labels.yml and labels, and you cannot have labels.yaml and labels as options for the user.

By all means we can simplify the look-up code here - i.e. attempt to pre-cache the filenames using a watcher informed system etc. but it's not correct to pass the filename up to the user.

I do not believe that the filename should be the API and I don't believe that others intend to make it so. #23749 makes the filename part of both the internal API and /api/v1.

#23749 is breaking and worse it's not even being acknowledged that this is the case.

wxiaoguang · 2023-03-29T16:29:33Z

Actually, labels.yaml that's just a name, the name of the label set template. The API caller calls the template=Advanced.yaml to create their repo, that's pretty clear and consistent IMO. I do not understand why Advanced.yaml shouldn't appear in the API parameters. It is somewhat breaking, but it's worthy if it's necessary , the "label" package expects to see file names, see comment below. (Update: after 964d81b, only Advanced is used)

It doesn't make sense to use complex patches to avoid some trivial breakings. I have made many "breaking" PRs, IMO there are all good to the whole project.

Update: to address the "breaking" concern, the commit 964d81b only uses lables for the name.

And for "I do not believe that the filename should be the API and I don't believe that others intend to make it so.":

Actually, the "label" package expects to see "file name", you can find such design in old code, including the TemplateFile in the error struct. New code made a mess, so we need to clarify the design, avoid guessing.

wxiaoguang · 2023-03-29T16:32:34Z

And now #23749 has already done de-duplication, it only serves one name for duplicated main names. To avoid breaking, we can just make the code look up the "name" in the "LabelTemplateFiles", then 100% nothing breaks.

wxiaoguang · 2023-03-29T16:54:24Z

964d81b

This commit addresses your "breaking" concern, but still strictly follows the no-guessing rule. And it has tests to cover the priority logic.

…-legacy-for-yaml-templates

zeripath · 2023-04-07T17:07:54Z

I give up

Fix #23715 Other related PRs: * #23717 * #23716 * #23719 This PR is different from others, it tries to resolve the problem fundamentally (and brings more benefits) Although it looks like some more lines are added, actually many new lines are for tests. ---- Before, the code was just "guessing" the file type and try to parse them. <details> ![image](https://user-images.githubusercontent.com/2114189/228002245-57d58e27-1078-4da9-bf42-5bc0b264c6ce.png) </details> This PR: * Always remember the original option file names, and always use correct parser for them. * Another benefit is that we can sort the Label Templates now (before there was a map, its key order is undefined) ![image](https://user-images.githubusercontent.com/2114189/228002432-931b9f18-3908-484b-a36b-04760c9ad132.png)

…23749) Fix go-gitea#23715 Other related PRs: * go-gitea#23717 * go-gitea#23716 * go-gitea#23719 This PR is different from others, it tries to resolve the problem fundamentally (and brings more benefits) Although it looks like some more lines are added, actually many new lines are for tests. ---- Before, the code was just "guessing" the file type and try to parse them. <details> ![image](https://user-images.githubusercontent.com/2114189/228002245-57d58e27-1078-4da9-bf42-5bc0b264c6ce.png) </details> This PR: * Always remember the original option file names, and always use correct parser for them. * Another benefit is that we can sort the Label Templates now (before there was a map, its key order is undefined) ![image](https://user-images.githubusercontent.com/2114189/228002432-931b9f18-3908-484b-a36b-04760c9ad132.png) # Conflicts: # modules/label/parser.go # routers/web/org/setting.go # routers/web/repo/issue_label.go # templates/repo/create.tmpl # templates/repo/issue/labels/label_load_template.tmpl

zeripath added type/bug outdated/backport/v1.19 This PR should be backported to Gitea 1.19 labels Mar 26, 2023

zeripath added this to the 1.20.0 milestone Mar 26, 2023

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 26, 2023

zeripath mentioned this pull request Mar 26, 2023

yaml/yml label file if/else guards #23717

Closed

zeripath force-pushed the fix-23715-do-not-call-legacy-for-yaml-templates branch from 5fef66a to 5f19f54 Compare March 27, 2023 13:26

wxiaoguang reviewed Mar 27, 2023

View reviewed changes

zeripath added 3 commits March 27, 2023 15:43

Ensure that legacy formats within customPath still override builtins

f670840

Signed-off-by: Andrew Thornton <art27@cantab.net>

oops

4201a37

Signed-off-by: Andrew Thornton <art27@cantab.net>

fix static build

07bac3f

Signed-off-by: Andrew Thornton <art27@cantab.net>

wxiaoguang mentioned this pull request Mar 27, 2023

Make label templates have consistent behavior and priority #23749

Merged

Merge remote-tracking branch 'origin/main' into fix-23715-do-not-call…

e43bd49

…-legacy-for-yaml-templates

zeripath closed this Apr 7, 2023

GiteaBot removed this from the 1.20.0 milestone Apr 23, 2023

go-gitea locked as resolved and limited conversation to collaborators Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zeripath commented Mar 26, 2023

zeripath commented Mar 26, 2023

wxiaoguang commented Mar 26, 2023 •

edited

Loading

codecov-commenter commented Mar 26, 2023 •

edited

Loading

eeyrjmr commented Mar 26, 2023

wxiaoguang commented Mar 26, 2023 •

edited

Loading

zeripath commented Mar 27, 2023

wxiaoguang Mar 27, 2023 •

edited

Loading

zeripath Mar 27, 2023

wxiaoguang Mar 27, 2023

wxiaoguang Mar 27, 2023

zeripath Mar 27, 2023

wxiaoguang Mar 27, 2023 •

edited

Loading

zeripath commented Mar 28, 2023

wxiaoguang commented Mar 28, 2023

wxiaoguang commented Mar 28, 2023 •

edited

Loading

eeyrjmr commented Mar 28, 2023

wxiaoguang commented Mar 28, 2023 •

edited

Loading

zeripath commented Mar 29, 2023

wxiaoguang commented Mar 29, 2023 •

edited

Loading

eeyrjmr commented Mar 29, 2023

wxiaoguang commented Mar 29, 2023

jolheiser commented Mar 29, 2023

wxiaoguang commented Mar 29, 2023

wxiaoguang commented Mar 29, 2023 •

edited

Loading

zeripath commented Mar 29, 2023 •

edited

Loading

wxiaoguang commented Mar 29, 2023 •

edited

Loading

wxiaoguang commented Mar 29, 2023

wxiaoguang commented Mar 29, 2023

zeripath commented Apr 7, 2023

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

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

Conversation

zeripath commented Mar 26, 2023

zeripath commented Mar 26, 2023

wxiaoguang commented Mar 26, 2023 • edited Loading

codecov-commenter commented Mar 26, 2023 • edited Loading

Codecov Report

eeyrjmr commented Mar 26, 2023

wxiaoguang commented Mar 26, 2023 • edited Loading

zeripath commented Mar 27, 2023

wxiaoguang Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

zeripath Mar 27, 2023

Choose a reason for hiding this comment

wxiaoguang Mar 27, 2023

Choose a reason for hiding this comment

wxiaoguang Mar 27, 2023

Choose a reason for hiding this comment

zeripath Mar 27, 2023

Choose a reason for hiding this comment

wxiaoguang Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

zeripath commented Mar 28, 2023

wxiaoguang commented Mar 28, 2023

wxiaoguang commented Mar 28, 2023 • edited Loading

eeyrjmr commented Mar 28, 2023

Option 1

Option 2

Option 3

wxiaoguang commented Mar 28, 2023 • edited Loading

zeripath commented Mar 29, 2023

wxiaoguang commented Mar 29, 2023 • edited Loading

eeyrjmr commented Mar 29, 2023

wxiaoguang commented Mar 29, 2023

jolheiser commented Mar 29, 2023

wxiaoguang commented Mar 29, 2023

wxiaoguang commented Mar 29, 2023 • edited Loading

zeripath commented Mar 29, 2023 • edited Loading

wxiaoguang commented Mar 29, 2023 • edited Loading

wxiaoguang commented Mar 29, 2023

wxiaoguang commented Mar 29, 2023

zeripath commented Apr 7, 2023

wxiaoguang commented Mar 26, 2023 •

edited

Loading

codecov-commenter commented Mar 26, 2023 •

edited

Loading

wxiaoguang commented Mar 26, 2023 •

edited

Loading

wxiaoguang Mar 27, 2023 •

edited

Loading

wxiaoguang Mar 27, 2023 •

edited

Loading

wxiaoguang commented Mar 28, 2023 •

edited

Loading

wxiaoguang commented Mar 28, 2023 •

edited

Loading

wxiaoguang commented Mar 29, 2023 •

edited

Loading

wxiaoguang commented Mar 29, 2023 •

edited

Loading

zeripath commented Mar 29, 2023 •

edited

Loading

wxiaoguang commented Mar 29, 2023 •

edited

Loading