Fixed incorrect depsdev getporject #1438
Conversation
- Fixed the issue of incorrect GetProject issue - Fixes guacsec#1413 Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
naveensrinivasan
force-pushed
the
naveen/fix-deps-dev-project-key
branch
from
916ad99
to
1843c45
Compare
| @@ -568,7 +567,7 @@ func (d *depsCollector) collectAdditionalMetadata(ctx context.Context, pkgType s | |||
| logger.Debugf("The project key was not found in the map: %v", projectReq.ProjectKey) | |||
| project, err = d.client.GetProject(ctx, projectReq) | |||
| if err != nil { | |||
| logger.Debugf("unable to get project for: %v, error: %v", projectReq.ProjectKey.Id, err) | |||
| logger.Infof("unable to get project for: %v, error: %v", projectReq.ProjectKey.Id, err) | |||
There was a problem hiding this comment.
nit: This query will still fail for various reasons, so I think it will be too verbose for Info level. Keeping it as Debug should be fine because #1422 will be merged soon.
| t.Errorf("NewDepsCollector() error = %v", err) | ||
| return | ||
| } | ||
| if err := c.collectAdditionalMetadata(context.Background(), tt.pkgType, tt.namespace, tt.name, tt.version, tt.pkgComponent); (err != nil) != tt.wantErr { |
There was a problem hiding this comment.
nit: use ctx defined above instead of context.Background()
| t.Errorf("NewDepsCollector() error = %v", err) | ||
| return | ||
| } | ||
| if err := c.collectAdditionalMetadata(context.Background(), tt.pkgType, tt.namespace, tt.name, tt.version, tt.pkgComponent); (err != nil) != tt.wantErr { |
There was a problem hiding this comment.
If retrieving the project fails, then collectAdditionalMetadata logs the error instead of returning it. As a result, this test doesn't really capture what is intended to be tested (i.e. that the d.client.GetProject call does not fail).
| // Based on the issue https://github.com/guacsec/guac/issues/1413, we need to ensure | ||
| // that the .git suffix is removed from the repository name. This is because some | ||
| // repository URLs might include the .git suffix which is not expected by certain endpoints. | ||
| m.Name = strings.TrimSuffix(m.Name, ".git") |
There was a problem hiding this comment.
Should we also trim .git after possibly removing the version? If there is a version specified after the .git then it won't be removed.
|
This pull request has been automatically marked as stale because it has not had recent activity (60 days of inactivity). |
|
Hi @naveensrinivasan just pinging on this from stale-bot notification - are you still looking to finish up this PR? |
|
This pull request has been automatically marked as stale because it has not had recent activity (60 days of inactivity). |
|
@naveensrinivasan should we close this or will you be finishing it up? Thanks |
Sorry I don't have cycles now. |
|
This pull request has been automatically marked as stale because it has not had recent activity (60 days of inactivity). |
|
This pull request has been automatically closed because there has been no activity for 90 days. |
|
Reopening as the fix may be needed. |
Description of the PR
PR Checklist
-sflag togit commit.make generatehas been runcollectsubprotobuf has been changed,make protohas been run