-
Notifications
You must be signed in to change notification settings - Fork 36
feat: add --ignore-paths support #1019
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
base: main
Are you sure you want to change the base?
Conversation
175b163 to
4c3ae02
Compare
flux_local/git_repo.py
Outdated
|
|
||
| kinds = [CLUSTER_KUSTOMIZE_KIND, CONFIG_MAP_KIND, SECRET_KIND] | ||
|
|
||
| def _normalize_patterns(patterns: list[str]) -> list[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in this section here and below seem extremely complicated for implementing this feature. It seems like it only needs to be like 5 line change. There are now 3 copies of essentially the same code, so I'd like to decline this, unless it can be 10x smaller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totes, yea admittedly I just vibed this out last night when i hit the issue of having yaml in a generated configmap being processed again. I'll rework this portion manually as yea it just vomited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran into the same problem it looks like codex did, since kustomize cfg grep doesn't support anything other than .krmignore, plumbing through to flux any ignore-paths fails on the kustomize call searching for the flux GitRepository. I first tried generating a temporary .krmignore but then this has the side effect that it could overwrite and existing one, which we could then look to merge in, but that then could accidentally get committed to a repo, so I abandoned that line.
I too then implemented the search in the case of ignores manually, but I believe a bit more manageable and maintainable, leveraging pathspec which was already a transitive dependency to walk the tree and apply the filters for us. I kept the change gated such that only if ignore-paths is specified will it use the python based search, if there are no ignore-paths it continues to use kustomize cfg grep.
If you'd like I can make the python walking the only one. I've tested it on a fairly large private repository and it completed the task around the same as shelling out to kustomize.
There is similar walking code in the flux-local diagnostics code, if you want I can look at combining the two into a single walking support with filtering, but I didn't eagerly do that to keep the diff smaller.
Add `--ignore-paths` flag to CLI and actions. Some repositories may contain yaml files that are not kustomize or cluster resources. Allow skipping those paths. Fixes: allenporter#211
4c3ae02 to
332d57f
Compare
|
|
||
| # Initialize git repository | ||
| repo = git.Repo.init(repo_path) | ||
| repo = git.Repo.init(repo_path, initial_branch='master') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locally test_git_repository_branch_reconciliation was failing for me since it was getting inited with main as the branch name, where the test expects it to be master.
Add
--ignore-pathsflag to CLI and actions.Some repositories may contain yaml files that are not kustomize or cluster resources. Allow skipping those paths.
Fixes: #211