fix: request license information from GitLab API#6593
Conversation
|
I have read the CLA Document and I hereby sign the CLA |
intelligent-ears
left a comment
There was a problem hiding this comment.
This PR doesn't add any tests verifying that the license field actually gets populated.There can be at least a unit test for gitlabProjectToProperties that confirms license is non-empty when the API response includes a license.
| ) (*gitlab.Project, error) { | ||
| projectURLPath, err := url.JoinPath("projects", url.PathEscape(upstreamID)) | ||
| projectURLPath = projectURLPath + "?license=true" | ||
| if err != nil { |
There was a problem hiding this comment.
The ?license=true is appended before the error check — the code appends the query param before checking if url.JoinPath returned an error. If JoinPath fails, projectURLPath would be an empty string and the append would produce just ?license=true. The correct order is:
projectURLPath, err := url.JoinPath("projects", url.PathEscape(upstreamID))
if err != nil {
return nil, fmt.Errorf("failed to join URL path for project using upstream ID: %w", err)
}
projectURLPath = projectURLPath + "?license=true"
In particular, we should probably update |
c9a27e7 to
dd9935e
Compare
evankanderson
left a comment
There was a problem hiding this comment.
Thanks for adding the tests! The test implementation brought up a couple questions (this is the learning process, so it's totally normal to hit these sorts of things).
| }, | ||
| }, | ||
| { | ||
| name: "repository succeeds", |
There was a problem hiding this comment.
Why change this name to empty-string?
| Path: "group", | ||
| }, | ||
| License: &gitlab.ProjectLicense{ | ||
| Name: "mit", // Changed Key to Name to match repository_properties.go |
There was a problem hiding this comment.
Will this comment make sense in 6 months?
| } | ||
|
|
||
|
|
||
| func TestGitlabProjectToProperties_License(t *testing.T) { |
There was a problem hiding this comment.
What does this test cover that the other test doesn't?
In general, extra tests add maintenance burden when refactoring code. That burden pays off when the tests cover additional important code cases, but redundant tests add more maintenance cost without commensurate benefits.
Summary
Provide a brief overview of the changes and the issue being addressed.
Explain the rationale and any background necessary for understanding the changes.
List dependencies required by this change, if any.
Fixes #(related issue)
Testing
Summary
This PR fixes an issue where the
gitlab/licenseentity property (RepoPropertyLicense) is always empty for GitLab repositories. The fetch logic ininternal/providers/gitlab/repository_properties.gohas been updated to correctly pass the?license=truequery parameter required by the GitLab API to include license details in the response.Fixes #6589
Testing
This is a straightforward API query parameter addition. The underlying
gitlabprovider package logic handles the mapping once the license payload is retrieved via the modified URL path.