fix(appset): update gitlab SCM provider to search on parent folder (#16253)

[prune998]

Fixes [#16253]

This PR solves the ISsue with Gitlab 17.7 (probably others) where using the ListTree API request and specifying a path to a file returns a 404.
This is a change in the API and prevent usage of files in the Gitlab SCM PathExist.

This PR changes the way we do the search, by looking for all individual entries in the parent folder, whatever we're looking for a file or a folder.

Pros

• we only query the API once
• we solve the 404 bug

Cons

• in case there's a ton of entities (folders and files) in the parent folder and we only care for a folder, we have to iterate over all the entities until we find the right one. This is already the case when looking for a file, so it's not a real behaviour change.

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• The title of the PR conforms to the Toolchain Guide
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.
• Optional. My organization is added to USERS.md.
• Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

[bunnyshell]

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

• 🚀 /bns:deploy to deploy the environment

[codecov]

Codecov Report

Attention: Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.

Project coverage is 55.25%. Comparing base (d23e6ac) to head (8974f93).

Files with missing lines	Patch %	Lines
applicationset/services/scm_provider/gitlab.go	78.57%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #21491      +/-   ##
==========================================
+ Coverage   53.35%   55.25%   +1.90%     
==========================================
  Files         337      337              
  Lines       56846    56837       -9     
==========================================
+ Hits        30330    31406    +1076     
+ Misses      23876    22744    -1132     
- Partials     2640     2687      +47     

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

[crenshaw-dev]

Thanks for the PR! Code LGTM. Could we get some test coverage here? https://github.com/argoproj/argo-cd/blob/master/applicationset/services/pull_request/gitlab_test.go

[prune998]

I'm not sure I can add tests on some remote API changes... the function is already tested and detects all the cases. I don't see the point of testing this 404 thing. The RepoHasPath function had a bad design that was exposed when Gitlab changed it API behaviour. Now we're just not triggering it anymore...

[prune998]

I had a second look, and while I could create a case like:

		case "/api/v4/projects/27084533/repository/tree?path=argocd/file.txt&ref=master":
			w.WriteHeader(http.StatusNotFound)

There's no way to trigger it by calling RepoHasPath because the patch prevents calling gitlab tree with a file as path ...

[prune998]

@crenshaw-dev how to get that ported to older releases as it's a fix for a bug linked to Gitlab, not a real bug in Argo, but impact mostly all previous versions ?

[crenshaw-dev]

There are two existing tests which should cover this code path:

		case "/api/v4/projects/27084533/repository/tree?path=argocd&ref=master":
			_, err := io.WriteString(w, `[{"id":"ca14f2a3718159c74572a5325fb4bfb0662a2d3e","name":"ingress.yaml","type":"blob","path":"argocd/ingress.yaml","mode":"100644"},{"id":"de2a53a73b1550b3e0f4d37ea0a6d878bf9c5096","name":"install.yaml","type":"blob","path":"argocd/install.yaml","mode":"100644"}]`)
			if err != nil {
				t.Fail()
			}
		case "/api/v4/projects/27084533/repository/tree?path=.&ref=master":
			_, err := io.WriteString(w, `[{"id":"f2bf99fa8f7a27df9c43d2dffc8c8cd747f3181a","name":"argocd","type":"tree","path":"argocd","mode":"040000"},{"id":"68a3125232e01c1583a6a6299534ce10c5e7dd83","name":"manifests","type":"tree","path":"manifests","mode":"040000"}]`)
			if err != nil {
				t.Fail()
			}

I think we should 1) modify the existing tests to reproduce the bug (i.e. return a 404 when a file path is specified) and 2) add new tests to reflect the new API call and response.

how to get that ported to older releases

We'll cherry-pick it and cut patch releases. A bug caused by an external change is still a bug as far as patching goes imo.

[prune998]

we can't reproduce the bug... because the patch removes the call that was returning the 404... and the new code never calls the gitlab API with a file path, only with a directory path...

I can add the test I provided just above, which would replicate the Gitlab API (new) behaviour... but the code just never calls the API this way anymore...

[crenshaw-dev]

we can't reproduce the bug

You can run the old code against the new test and confirm that the test fails. Then we can leave that test in place to assure that no one re-introduces code that assumes the old API behavior.

the new code never calls the gitlab API with a file path, only with a directory path

Yep, we should have tests that handle the directory path case. I'm not sure exactly what the API call path will look like, but our tests should have that path and a corresponding test response.

[prune998]

OK I think I got it :)
I added the response case, that old code should trigger, while new code will not.

It is cumbersome as the file does not exist and file exists test cases should both fail with a 404 when using gitlab API 17.7. But I wanted to keep them as is... so I added another test that will fail with the new API and the old code.

[crenshaw-dev]

LGTM! I'm still a bit wary of any change that could cause app deletion... how would you feel about just cherry-picking to 2.14 for now and giving it a few weeks before cherry-picking back further?

[crenshaw-dev]

/cherry-pick release-2.14

[prune998]

as you wish. But this change should not remove any app... works the same, it's just not searching for the directory straight away, but parses all the files/folders in the parent folder... there's no behaviour change.

[crenshaw-dev]

I agree, but also I don't entirely trust my eyes or even the tests. I've been unpleasantly surprised too many times. :-)

[dfl-aeb]

Thank you for addressing the issue! Will there be a patch release to include this fix sooner, or will it only be part of version 2.14 scheduled for February?

[crenshaw-dev]

@dfl-aeb we'll ship it in the next 2.14 RC. If things look good after releasing the bugfix in 2.14, we can consider cherry-picking to other supported releases.

[dfl-aeb]

We're fine with the 2.14 RC thx