feat: add support for filtering apps by path and files #21126

slashexx · 2024-12-11T08:23:06Z

Fixes #21052

Checklist:

Signed-off-by: slashexx <[email protected]>

bunnyshell · 2024-12-11T08:23:15Z

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

🔵 /bns:start to start the environment
🚀 /bns:deploy to redeploy the environment
❌ /bns:delete to remove the environment

Signed-off-by: slashexx <[email protected]>

codecov · 2024-12-11T09:07:19Z

Codecov Report

Attention: Patch coverage is 72.97297% with 10 lines in your changes missing coverage. Please review.

Project coverage is 53.46%. Comparing base (bcf2143) to head (e43f346).

Files with missing lines	Patch %	Lines
cmd/argocd/commands/app.go	0.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #21126      +/-   ##
==========================================
- Coverage   55.45%   53.46%   -1.99%     
==========================================
  Files         339      339              
  Lines       57196    57233      +37     
==========================================
- Hits        31718    30602    -1116     
- Misses      22812    23998    +1186     
+ Partials     2666     2633      -33

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: slashexx <[email protected]>

nitishfy

Is this PR ready for review? Also, can you fix the CI checks?

Signed-off-by: slashexx <[email protected]>

slashexx · 2025-01-21T10:20:36Z

@nitishfy Thanks for following up, apologies for the delay in fixing this.

The CI checks should now work fine, I've also marked this PR as ready for review, PTAL !

nitishfy · 2025-01-23T11:24:47Z

util/argo/argo.go

+func FilterByPath(apps []argoappv1.Application, path string) []argoappv1.Application {
+	filteredApps := make([]argoappv1.Application, 0)
+	for _, app := range apps {
+		if app.Spec.Source != nil && (app.Spec.Source.Path == path || strings.HasPrefix(app.Spec.Source.Path, path+"/")) {


Suggested change

if app.Spec.Source != nil && (app.Spec.Source.Path == path || strings.HasPrefix(app.Spec.Source.Path, path+"/")) {

if app.Spec.Source != nil && (app.Spec.Source.Path == path || strings.HasPrefix(app.Spec.Source.Path, path+"/")) {

You'd also need to filter the apps that consist of multiple sources. Right now, it only checks for a single source application.

nitishfy · 2025-01-23T11:25:24Z

util/argo/argo.go

+func FilterByPath(apps []argoappv1.Application, path string) []argoappv1.Application {
+	filteredApps := make([]argoappv1.Application, 0)
+	for _, app := range apps {
+		if app.Spec.Source != nil && (app.Spec.Source.Path == path || strings.HasPrefix(app.Spec.Source.Path, path+"/")) {


Suggested change

if app.Spec.Source != nil && (app.Spec.Source.Path == path || strings.HasPrefix(app.Spec.Source.Path, path+"/")) {

if app.Spec.Source != nil && app.Spec.Source.Path == path {

why are you checking for strings.HasPrefix() here?

Hi @nitishfy , apologies I'm a bit new around here.

I've put it like that so it compares the exact path along with subdirectories and all ?

Is it not required ?

You'd need to check for absolute and relative path here instead of just checking if a path consist of /. Isn't this the intended behaviour? cc @agaudreault

nitishfy · 2025-01-23T11:30:06Z

util/argo/argo.go

+func FilterByPath(apps []argoappv1.Application, path string) []argoappv1.Application {
+	filteredApps := make([]argoappv1.Application, 0)
+	for _, app := range apps {
+		if app.Spec.Source != nil && (app.Spec.Source.Path == path || strings.HasPrefix(app.Spec.Source.Path, path+"/")) {


Suggested change

if app.Spec.Source != nil && (app.Spec.Source.Path == path || strings.HasPrefix(app.Spec.Source.Path, path+"/")) {

if app.Spec.Source != nil && (app.Spec.Source.Path == path || strings.HasPrefix(app.Spec.Source.Path, path+"/")) {

Checking for a path is simply not sufficient. You should also check for a absolute/relative path too. For eg. in any setup, I should be simply able to pass the file path to a file that exists locally and it should internally check if any application is using that specified path for sync operation.

@agaudreault Isn't this what we plan to do? Otherwise with the current setup we just pass an argument to the path flag and check if that matches with the source path.

nitishfy

Thanks for working on this. I've not reviewed the --file flag implementation yet. However, i think a lot of things can be improved for the --path flag implementation.

slashexx · 2025-01-24T11:36:40Z

Thanks for working on this. I've not reviewed the --file flag implementation yet. However, i think a lot of things can be improved for the --path flag implementation.

Thanks for reviewing this PR, I'll make the required changes and change --file flag accordingly as well.

Signed-off-by: slashexx <[email protected]>

Added support for filtering by path and files

d3620a9

Signed-off-by: slashexx <[email protected]>

slashexx requested a review from a team as a code owner December 11, 2024 08:23

slashexx marked this pull request as draft December 11, 2024 08:23

Fixed formatting

3e4480d

Signed-off-by: slashexx <[email protected]>

slashexx and others added 5 commits December 11, 2024 16:29

Merge branch 'master' into feature/pathfilefilter

d8131cb

Add tests

b83fd3b

Signed-off-by: slashexx <[email protected]>

Proper fmt

17029bf

Signed-off-by: slashexx <[email protected]>

Merge branch 'master' into feature/pathfilefilter

7043fe5

Merge branch 'master' into feature/pathfilefilter

d1581eb

nitishfy reviewed Jan 21, 2025

View reviewed changes

nitishfy self-assigned this Jan 21, 2025

slashexx and others added 7 commits January 21, 2025 13:09

Merge branch 'argoproj:master' into feature/pathfilefilter

320edc0

fix: fixed the testcases and rewrote logic for filter functions

ddc814d

Signed-off-by: slashexx <[email protected]>

fix: ran go fmt ./...

82f24fc

Signed-off-by: slashexx <[email protected]>

fix: fix formatting

5e51265

Signed-off-by: slashexx <[email protected]>

fix: remove extra line

0940a4a

Signed-off-by: slashexx <[email protected]>

feat: added to doc

2096f89

Signed-off-by: slashexx <[email protected]>

feat: removed extra dot

d1b5faf

Signed-off-by: slashexx <[email protected]>

slashexx marked this pull request as ready for review January 21, 2025 10:20

slashexx requested a review from a team as a code owner January 21, 2025 10:20

slashexx added 2 commits January 21, 2025 19:04

Merge branch 'master' into feature/pathfilefilter

5279676

Merge branch 'master' into feature/pathfilefilter

b4515f4

nitishfy reviewed Jan 23, 2025

View reviewed changes

nitishfy suggested changes Jan 23, 2025

View reviewed changes

nitishfy mentioned this pull request Jan 23, 2025

REQUEST: Promotion to Reviewer for nitishfy argoproj/argoproj#349

Open

6 tasks

Merge branch 'master' into feature/pathfilefilter

e43f346

agaudreault self-assigned this Jan 29, 2025

slashexx and others added 2 commits February 1, 2025 00:18

Merge branch 'master' into feature/pathfilefilter

e483a5e

fix: refactored the path filter to include relative paths

28f71bf

Signed-off-by: slashexx <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for filtering apps by path and files #21126

feat: add support for filtering apps by path and files #21126

slashexx commented Dec 11, 2024 •

edited

Loading

bunnyshell bot commented Dec 11, 2024 •

edited

Loading

codecov bot commented Dec 11, 2024 •

edited

Loading

nitishfy left a comment

slashexx commented Jan 21, 2025

nitishfy Jan 23, 2025

nitishfy Jan 23, 2025 •

edited

Loading

slashexx Jan 24, 2025

nitishfy Jan 24, 2025

nitishfy Jan 23, 2025

nitishfy left a comment

slashexx commented Jan 24, 2025 •

edited

Loading

	if app.Spec.Source != nil && (app.Spec.Source.Path == path \|\| strings.HasPrefix(app.Spec.Source.Path, path+"/")) {
	if app.Spec.Source != nil && (app.Spec.Source.Path == path \|\| strings.HasPrefix(app.Spec.Source.Path, path+"/")) {

	if app.Spec.Source != nil && (app.Spec.Source.Path == path \|\| strings.HasPrefix(app.Spec.Source.Path, path+"/")) {
	if app.Spec.Source != nil && app.Spec.Source.Path == path {

feat: add support for filtering apps by path and files #21126

Are you sure you want to change the base?

feat: add support for filtering apps by path and files #21126

Conversation

slashexx commented Dec 11, 2024 • edited Loading

bunnyshell bot commented Dec 11, 2024 • edited Loading

🔴 Preview Environment stopped on Bunnyshell

codecov bot commented Dec 11, 2024 • edited Loading

Codecov Report

nitishfy left a comment

Choose a reason for hiding this comment

slashexx commented Jan 21, 2025

nitishfy Jan 23, 2025

Choose a reason for hiding this comment

nitishfy Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

slashexx Jan 24, 2025

Choose a reason for hiding this comment

nitishfy Jan 24, 2025

Choose a reason for hiding this comment

nitishfy Jan 23, 2025

Choose a reason for hiding this comment

nitishfy left a comment

Choose a reason for hiding this comment

slashexx commented Jan 24, 2025 • edited Loading

slashexx commented Dec 11, 2024 •

edited

Loading

bunnyshell bot commented Dec 11, 2024 •

edited

Loading

codecov bot commented Dec 11, 2024 •

edited

Loading

nitishfy Jan 23, 2025 •

edited

Loading

slashexx commented Jan 24, 2025 •

edited

Loading