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

Normalize file paths in LoadPaths #169

Merged
merged 1 commit into from Aug 10, 2021
Merged

Conversation

jason-fugue
Copy link
Contributor

@jason-fugue jason-fugue commented Aug 10, 2021

This PR resolves #167. The issue was that a lot of our code produces 'clean' paths and we weren't accounting for that when we check whether or not we've already loaded a file. This was causing TF files to get loaded twice if the input path had a leading ./.

Copy link
Member

@jaspervdj-luminal jaspervdj-luminal left a comment

Choose a reason for hiding this comment

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

Sort of nitpicky but I think it's a better idea to keep the paths as they are as much as possible, and move this logic to AlreadyLoaded / loadedPaths?

@jason-fugue
Copy link
Contributor Author

Sort of nitpicky but I think it's a better idea to keep the paths as they are as much as possible, and move this logic to AlreadyLoaded / loadedPaths?

I was leaning towards doing it this way because the behavior seems more consistent afterwards. One example where it's inconsistent without that transformation is the behavior for directories with a leading ./ vs individual files:

# Pointing to ./infra_cfn/cloudformation.yaml
$ regula run ./infra_cfn/cloudformation.yaml

FG_R00092: IAM policies should not have full "*:*" administrative privileges [High]

  [1]: InvalidGroup01
       in ./infra_cfn/cloudformation.yaml:94:3

# Pointing to the ./infra_cfn directory. Somewhere down the line, os.ReadDir drops the leading ./, so the filepath becomes infra_cfn/cloudformation.yaml
$ regula run ./infra_cfn 

FG_R00092: IAM policies should not have full "*:*" administrative privileges [High]

  [1]: InvalidGroup01
       in infra_cfn/cloudformation.yaml:94:3

Preemptively applying filepath.Clean() feels a lot simpler and makes it so that the filepath looks like same whether you point to it directly or regula finds it.

Are you worried that folks would find it confusing that we dropped the ./ or do you think this could potentially cause some bugs?

Copy link
Member

@jaspervdj-luminal jaspervdj-luminal left a comment

Choose a reason for hiding this comment

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

More that it could cause bugs/inconveniences for users, when they would run regula run -f json X and then index/filter by X, especially if they are not using Go and their filepath library has a subtly different Clean. But I think your justification works; we can always change this if necessary.

@jason-fugue jason-fugue merged commit fc239c2 into master Aug 10, 2021
@jason-fugue jason-fugue deleted the fix/167/normalize-paths branch August 18, 2021 20:20
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.

[BUG] Bad interpretation of the path
2 participants