[PL-24911]: Make project name as optional param in Azure Repo APIs #173
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mohitg0795
reviewed
May 2, 2022
scm/driver/azure/azure.go
Outdated
| @@ -25,7 +25,7 @@ func New(uri, owner, project string) (*scm.Client, error) { | |||
| if !strings.HasSuffix(base.Path, "/") { | |||
| base.Path = base.Path + "/" | |||
| } | |||
| if owner == "" || project == "" { | |||
| if owner == "" { | |||
| return nil, fmt.Errorf("azure owner and azure project are required") | |||
There was a problem hiding this comment.
logline should be fixed accordingly.
tphoney
approved these changes
May 3, 2022
|
@DeepakPatankar looks good with the change, but you will need to fix the unit tests first. |
allofthesepeople
approved these changes
May 3, 2022
scm/driver/azure/repo.go
Outdated
| @@ -40,7 +40,12 @@ func (s *RepositoryService) FindPerms(ctx context.Context, repo string) (*scm.Pe | |||
| // List returns the user repository list. | |||
| func (s *RepositoryService) List(ctx context.Context, opts scm.ListOptions) ([]*scm.Repository, *scm.Response, error) { | |||
| // https://docs.microsoft.com/en-us/rest/api/azure/devops/git/repositories/list?view=azure-devops-rest-6.0 | |||
| endpoint := fmt.Sprintf("%s/%s/_apis/git/repositories?api-version=6.0", s.client.owner, s.client.project) | |||
| var endpoint string | |||
| if s.client.project != "" { | |||
There was a problem hiding this comment.
nit: prefer positive to negative conditionals as they're easier/quicker to understand.
tphoney
reviewed
May 4, 2022
There was a problem hiding this comment.
If a user creates a scm client and does not set the project, it will fail spectacularly for all calls except for List repositories. It does not feel like there is any protection against this. with the change to
if owner == "" || project == "" {
return nil, fmt.Errorf("azure owner and azure project are required")
}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For the list Repo api of the Azure repo. the project name is not a required parameter.In Harness, we support an Account type connectors which won’t contain the project name. When we are doing the test connection of this connector, we call the scm API to list all the repositories of an organization. Currently the scm code doesn’t allow the project name as null, which I fixed in this ticket