Conversation
|
Thanks for the PR! I will take a closer look in the coming days. Just briefly browsing the cmd/ change I noticed the naming of things in Azure Devops seems to colide with the naming of GitLab. Just to make sure I understand the terminology correctly. Is this how things are named?
So in Azure Devops the only levels are Repositories that can be within one project? There are no stacking projects, and no higher level (organizations etc)? |
|
Hey thanks for the question! I tried to make the naming in the code match the naming in Azure DevOps and you're right it does collide with GitLab. The chart you made of Github/GitLab/Azure DevOps is correct. Azure DevOps does have the concept of organizations, but each organization would have a different base url and is pretty much a totally separate entity. There are no stacked/sub projects either. |
lindell
left a comment
There was a problem hiding this comment.
Just had time to look at some initial code in cmd. If you want to TAL, please do, otherwise feel free to wait until I have time to look at the rest of the code.
|
Hey @lindell, no rush on the rest of the code review, but just wanted to let you know I pushed up a commit to address the comments so far. |
lindell
left a comment
There was a problem hiding this comment.
Hello again,
Nice that you had time to look at the first comment. I've had very little time to work on this the last week, but I've been able to go through about half of the PR. In general I think it's great since you are not usually working with Go.
I publish the new comments, but as the last time, please feel free to ignore them until the entire review is done if you want.
If there is one comment to take a look at, it's about getting repositories by name, and not fetching all of them.
| return nil, err | ||
| } | ||
|
|
||
| locationClient := location.NewClient(context.Background(), connection) |
There was a problem hiding this comment.
Using a context.Background here does mean that this request will not be cancelable (by pressing Ctrl+C or similar).
I think we should either create a new client for each time we need it, with a cache (cache seems to already be implemented internally in the library). But with a help method, func (a *AzureDevOps) connection(ctx context.Context) .
Or make sure we send in a context in the creation.
There was a problem hiding this comment.
I think I understand what you mean, but let me make sure. Are you suggesting I defer creating the clients until I actually need to use them inside the various methods: ForkRepository, GetRepositories, etc., pass the context into the NewClient method at that time, and store it on the *AzureDevOps reference?
There was a problem hiding this comment.
Yes, that should probably be the way to do it 🙂
| projectMap := make(map[string]uuid.UUID) | ||
|
|
||
| if fork { | ||
| projects, err := coreClient.GetProjects(context.Background(), core.GetProjectsArgs{}) |
There was a problem hiding this comment.
Will this always return all projects? Even if there are thousands of them? If not I think it might be cleaner to get the name when needed (with the same cache structure), slightly more inefficient in some scenarios, and slightly less in others (with very new forks).
There was a problem hiding this comment.
Yeah, that will return all projects even if there are thousands. Are you thinking I should change this to a search by project name on demand?
There was a problem hiding this comment.
Yes, I think that would be the way to do it if the API allows
| } | ||
|
|
||
| func (a *AzureDevOps) getRepos(ctx context.Context) (*[]git.GitRepository, error) { | ||
| allRepos, err := a.gitClient.GetRepositories(ctx, git.GetRepositoriesArgs{}) |
There was a problem hiding this comment.
There is no way of getting a repository by name? (project + name)
Will skip to review this function until I know.
There was a problem hiding this comment.
I looked and there is a way of getting repositories by name as long as you also have the project name
There was a problem hiding this comment.
In that case (if we can avoid fetching all repositories in the entire org). We should instead do a fetching for only the projects / repositories defined.
…o add-azuredevops
| return err | ||
| } | ||
|
|
||
| newObjectID := "0000000000000000000000000000000000000000" |
There was a problem hiding this comment.
Please add a comment of what is happening here
There was a problem hiding this comment.
Added a comment but let me know if it's not sufficient. I pulled this value from the docs here: https://learn.microsoft.com/en-us/rest/api/azure/devops/git/refs/update-refs?view=azure-devops-rest-7.1&tabs=HTTP
There was a problem hiding this comment.
Please add the link to the docs as well and we should be good here
|
Tried to review through vscode, apparently it posts every comment and not together. Sorry if this was spammy to you. |
lindell
left a comment
There was a problem hiding this comment.
I'm now done with the review, thanks again for the PR!
Please let me know if you have any follow up questions, or if you need any assistance with fixing any of the comments. I can't guarantee any timeline, but I'm happy to help.
| if err == nil { | ||
| return a.convertRepo(existingFork), nil | ||
| } | ||
|
|
There was a problem hiding this comment.
This will not be corect the correct behaviour if the error is for example a network error. (A new fork will be requested).
Something similar to this need to be made:
| if err == nil { | |
| return a.convertRepo(existingFork), nil | |
| } | |
| if err == nil { | |
| return a.convertRepo(existingFork), nil | |
| } else if !errors.Is(err, core.NotFoundError) { | |
| return nil, err | |
| } |
Note: I have not verified how to check notfound errors (core.NotFoundError does not exist) with this API. But it can either be a constant error (use errors.Is), or a type error (use errors.As)
There was a problem hiding this comment.
I found a way to check not found errors that I think makes sense and pushed that up. Let me know if you think I should do something else please.
| np = r.ownerName | ||
| } | ||
|
|
||
| repoName := r.name + "." + strings.ReplaceAll(currentUser, " ", ".") |
There was a problem hiding this comment.
Forks can only be made to the current user in Azure devops? Not to projects (like to organizations in GitHub)
There was a problem hiding this comment.
Forks can actually only be made to Projects in Azure devops. In the lines above, I am appending the username to the end of the repo name so that multiple forks of the repo can be forked into the same project. The project to fork the repo into gets passed into this method through the newProject variable.
There was a problem hiding this comment.
Might this behavior be something users want to configure? I'm fine if you want to start with this, and potentially add it later.
Co-authored-by: Johan Lindell <johan@lindell.me>
…itter into add-azuredevops
Co-authored-by: Johan Lindell <johan@lindell.me>
|
Thank you very much for the thorough PR and being kind to my go code! Also, sorry it took so long for me to get back to this. I'll try and get any remaining changes fixed much faster. Any changes I pushed up I closed the conversation for and I left open any conversations that seemed like further discussion might be needed. I think we're getting close on this! |
lindell
left a comment
There was a problem hiding this comment.
Sorry for the long review time. Got it in the beginning of a longer vacation, and had forgot about it when I came back. Should be faster to respond to any subsequent messages.
| return nil, err | ||
| } | ||
|
|
||
| locationClient := location.NewClient(context.Background(), connection) |
There was a problem hiding this comment.
Yes, that should probably be the way to do it 🙂
| projectMap := make(map[string]uuid.UUID) | ||
|
|
||
| if fork { | ||
| projects, err := coreClient.GetProjects(context.Background(), core.GetProjectsArgs{}) |
There was a problem hiding this comment.
Yes, I think that would be the way to do it if the API allows
| } | ||
|
|
||
| func (a *AzureDevOps) getRepos(ctx context.Context) (*[]git.GitRepository, error) { | ||
| allRepos, err := a.gitClient.GetRepositories(ctx, git.GetRepositoriesArgs{}) |
There was a problem hiding this comment.
In that case (if we can avoid fetching all repositories in the entire org). We should instead do a fetching for only the projects / repositories defined.
| return err | ||
| } | ||
|
|
||
| newObjectID := "0000000000000000000000000000000000000000" |
There was a problem hiding this comment.
Please add the link to the docs as well and we should be good here
| np = r.ownerName | ||
| } | ||
|
|
||
| repoName := r.name + "." + strings.ReplaceAll(currentUser, " ", ".") |
There was a problem hiding this comment.
Might this behavior be something users want to configure? I'm fine if you want to start with this, and potentially add it later.
|
Would it be possible to finish this? :) I need to update a PR template in multiple Azure DevOps repos ;) If you need any assistance I could help :) |
|
How can we finish this? |
|
Could this be expedited? ADO support is required for us to bulk update! |
What does this change
This adds a new VersionController implementation for Azure DevOps
For example if it introduces a new flag or modifies a commands output, give an example of you running the command and showing real output here.
Redacted Command
Redacted Output
What issue does it fix
Closes #475
If there is not an existing issue, please make sure we have context on why this change is needed. .
Notes for the reviewer
go is not my main language, but hopefully this is pretty close to what you would be looking for in a new VersionController for AzureDevops. I tried my best to follow existing implementations, but I'm happy to make any changes!
Checklist