-
Notifications
You must be signed in to change notification settings - Fork 48
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
Doublestar excludes doesn't seem to work: #97
Comments
Hi @esn89, thanks for opening an issue! Firstly, that error: If you quoted it in the manner you mentioned in the last sentence, like As for finding a config that will work, I messed with something locally and I have something that seems to work. Firstly, my folder structure was that I had a
This configuration caused yamlfmt to properly ignore the yaml files in |
Thanks, let me give this one a try :) |
Thanks for the detailed explanation, @braydonk. It works. |
Awesome! Enjoy 🚀 |
@braydonk
For the .gitlab-ci.yml here:
|
I'm assuming this is on Windows with a path like This seems to be a bug in the doublestar library yamlfmt uses. Absolute paths on Windows don't appear to match properly, or there's some edge case to them that I don't understand. I'll look into it. As a workaround, I tried just doing |
This is on MacOS. I am wondering if I need to explicitly install the doublestar go package? I have it set up like this, (in case you're also using Null-ls):
|
Shouldn't need to install the doublestar Go package. It's bundled in directly with the yamlfmt binary. I don't have a Mac machine to test myself, but I have observed it on Windows at least. I'll look into it, chances are since it's happening on Mac as well it's something in the general package and not platform specific. |
The issue was that I wasn't matching using absolute paths in doublestar mode. This should work in the next release. |
Thank you @braydonk :) Looking forward. |
Hey @braydonk and @badouralix, hate to be a nag, but this is not working with my current config:
The file:
The contents:
The command I ran:
On Mac OS 13.3 |
I faced a similar issue with |
That's very frustrating, I don't know what could be happening. I tested this on Linux and Windows and haven't found a scenario where it doesn't work as it should. It feels very unlikely that it's something Mac specific, but I can't say for sure without having one to test. Another possibility is that this has something to do with using yamlfmt as a standalone binary, which is something I don't test extensively because I never expect it to have any difference. I'll try that and see if I get any different results. Sorry this has been painful to get working, I really thought it was fixed. Hopefully we can get to the bottom of this. |
Even as a standalone binary on Linux and Windows my standalone reproducer worked. If someone with a Mac could give this exact scenario a try and see if there's a different result that would be great.
doublestar: true
exclude:
- "/Users/username/**/x.yaml"
a:
b: 1
When I use this setup with the
When I comment out the
I think this scenario should be similarly matching the scenario described above. I'm hoping for someone to replicate this exact version of the scenario on Mac just so I can confirm whether my experiment is flawed or there's genuinely a difference on Mac. |
Never mind, I forgot to add
And after commenting the
It would be nice to have @esn89 would you be able to run this command please ? go version -m $(which yamlfmt) Here is what I have on an install with /opt/homebrew/bin/yamlfmt: go1.20.2
path github.com/google/yamlfmt/cmd/yamlfmt
mod github.com/google/yamlfmt (devel)
dep github.com/RageCage64/multilinediff v0.2.0 h1:yNSpSF5NXIrmo6bRIgO4Q0g7SXqFD4j/WEcBE+BdCFY=
dep github.com/bmatcuk/doublestar/v4 v4.6.0 h1:HTuxyug8GyFbRkrffIpzNCSK4luc0TY3wzXvzIZhEXc=
dep github.com/braydonk/yaml v0.7.0 h1:ySkqO7r0MGoCNhiRJqE0Xe9yhINMyvOAB3nFjgyJn2k=
dep github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
dep github.com/mitchellh/mapstructure v1.5.0 h1:jeMsZIYE/09sWLaz43PL7Gy6RuMjD2eJVyuac5Z2hdY=
build -buildmode=exe
build -compiler=gc
build -trimpath=true
build CGO_ENABLED=1
build GOARCH=arm64
build GOOS=darwin |
Here you are @badouralix
|
Your version of git clone -b debug-issue-97 https://github.com/badouralix/google-yamlfmt
cd google-yamlfmt
# Here replace the usual `yamlfmt ...` with `go run ./cmd/yamlfmt ...`
go run ./cmd/yamlfmt -conf=/Users/ev/.config/nvim/.yamlfmt ~/Documents/Projects/supercool/.gitlab-ci.yml The new debug lines start with |
@badouralix I'm experiencing the same issue with doublestar (fails on my Apple M1 running MacOS Ventura 13.2.1 and also on our Ubuntu 22.04 Github runner). I tried your branch with the following:
the contents of
and Here are the logs:
Even with a relative path this fails, config file:
Logs:
Hope this helps, everything was working fine for us up until v0.9.0 (rolling back to v0.8.0 works, but we want to take advantage of the content-based exclusions that were added) |
@rhbuf in your case, the excluded file is
Maybe this doublestar: true
exclude:
- "**/x.yaml"
- "**/x.yml" But now we need to investigate why it was working fine with v0.8.0 😵💫 |
@badouralix sorry my bad - I've amended my
This dummy example aside, my main issue was that I had a
and was running:
(truncated) logs:
This works fine with v0.8.0 and fails with many errors from files under |
I see, it seems the behavior changed when this diff was introduced in https://github.com/google/yamlfmt/pull/105/files#diff-64650b9066f9ed386546a3f7db1cc7c3a9bb4f422970d7506dd6b051b797f010 - match, err := doublestar.Match(filepath.Clean(pattern), path)
+ match, err := doublestar.PathMatch(filepath.Clean(pattern), absPath) Basically @rhbuf a quick fix for you would be to use this # yamlfmt mangles helm charts and throws errors,
# `helm lint` should be used for linting charts
exclude:
- **/data/k8s/charts/** @braydonk would you be able to clarify the need for an absolute path instead of the regular path ? |
Thanks for the help working through this @badouralix I have been sick and not looking at GitHub this weekend. The change to absolute path was because previously absolute paths wouldn't match on every circumstance. If the collected path was like this:
And the exclude path was like this:
Then it would think the collected path is not a match for the exclusion pattern. The only way that made sense was to get the absolute path of the collected path. What I didn't realize is that I just inverted the problem. This prompted me to take a look, and I realized that I have been skipping the Doublestar Path Collector test suite unintentionally for a long time. Just tried re-enabling those tests and they are all broken. I apologize for all the weird stuff I've introduced here trying to fix this without realizing what I did. I'm going to need to re-enable that test suite and then come up with a solution that actually works properly for this. Thank you everyone for your patience. Work has been busy, so I haven't been able to spend any work time on this recently. This will probably need to wait until I have an evening available to clean all this stuff up. Side note, yes I really should have a |
Doublestar tests have been disabled for quite a long time and I honestly forget why they were. Regardless, they were missing some features to be properly robust anyway. This PR re-enables the tests and adds two test cases relevant to google#97 that will be skipped individually for now.
Doublestar tests have been disabled for quite a long time and I honestly forget why they were. Regardless, they were missing some features to be properly robust anyway. This PR re-enables the tests and adds two test cases relevant to google#97 that will be skipped individually for now.
Doublestar tests have been disabled for quite a long time and I honestly forget why they were. Regardless, they were missing some features to be properly robust anyway. This PR re-enables the tests and adds two test cases relevant to #97 that will be skipped individually for now.
The PR I just merged re-enables the tests, and features a couple new test cases that demonstrate what I believe to be the root of the problem. If you mix relative and absolute paths with your include and exclude patterns, then |
Here is the debug log from that command:
Here is the
So far, it looks okay. The gitlab-ci.yaml wasn't touched. |
I decided to make it clear in the docs that this only works when both paths are absolute or both paths are relative, and to suggest that relative paths be used in most circumstances. It is probably a solvable problem to make this work in cases where some paths are absolute and some are relative, however with the limited time I have for this project these days I don't think I'm going to be able to make time for it. 😞 |
I have this:
And I am doing this to prevent the yamlfmt from formatting my
templates/
folder that are in my helm charts, since it messes everything up.However, it keeps complaining that:
2023/03/24 14:38:02 yaml: line 3: did not find expected alphabetic or numeric character
.So I put line 3 in quotes
"- **/templates/**"
and now the error goes away. However, it doesn't respect my excludes and continues to format things in thetemplates/
folder.The text was updated successfully, but these errors were encountered: