incubator-devlake icon indicating copy to clipboard operation
incubator-devlake copied to clipboard

feat: rewrite Azure DevOps plugin

Open mr-ks opened this issue 1 year ago • 4 comments

⚠️ Pre Checklist

Please complete ALL items in this checklist, and remove before submitting

  • [x] I have read through the Contributing Documentation.
  • [x] I have added relevant tests.
  • [x] I have added relevant documentation.
  • [x] I will add labels to the PR, such as pr-type/bug-fix, pr-type/feature-development, etc.

Summary

As discussed in #2604 this is a revamped version of the Python Azure DevOps Plugin, originally located at ../../python/plugins/azuredevops. Please note, that the current implementation lacks a database migration script to retain the data collected by the Python plugin. As a result, the plugin has been temporarily excluded from the go plugin build process to prevent conflicts with the existing Python version.

PLUGINS=$(find $PLUGIN_SRC_DIR/* -maxdepth 0 -type d -not -name core -not -name helper -not -name logs -not -empty -not -name azuredevops)

Please be careful and only activate the plugin if you know what you do. Once activated following tool tables will be dropped. The data aggregated in the domain tables will remain untouched which can leave your database in an undesired state.

"_raw_azuredevops_builds",
"_raw_azuredevops_gitpullrequestcommits",
"_raw_azuredevops_gitpullrequests",
"_raw_azuredevops_jobs"

"_tool_azuredevops_builds",
"_tool_azuredevops_gitpullrequestcommits",
"_tool_azuredevops_gitpullrequests",
"_tool_azuredevops_gitrepositories",
"_tool_azuredevops_gitrepositoryconfigs",
"_tool_azuredevops_jobs"

Does this close any open issues?

No.

Other Information

⚠️ Access to Service Connections has been removed as they usually contain sensitive security information.

Any other information that is important to this PR.

mr-ks avatar Feb 11 '24 16:02 mr-ks

@klesh apologies for including almost 100 files in this pull request. The primary objective of this initial submission is to achieve feature parity with the Python plugin. However, I have omitted the remote repositories from the service connections. Additionally, we need to create a migration plan if we want to phase out the Python plugin. Any suggestions on that?

mr-ks avatar Feb 11 '24 16:02 mr-ks

@mr-ks No worries about the files, I agree with you that the PR should implement whatever the current Python plugin does.

I think we can make it a new plugin alongside the Python one because it would be easier to code and it is acceptable for users to move connections from one to another.

klesh avatar Feb 19 '24 03:02 klesh

@klesh Do you have any suggestions for the naming? What do you think of using azuredevops_v2 as an identifier for the database tables and azuredevops-v2 for the APIs? E.g.:

Database
_tool_azuredevops_v2_pull_requests

API
api/plugins/azuredevops-v2/connections/...

mr-ks avatar Feb 20 '24 17:02 mr-ks

Maybe putting the go into the name would help to signify that it differs from the Python one.

klesh avatar Feb 22 '24 01:02 klesh

FWIW I had run into a problem with the python version failing to let me link more than 30 or so repos. so I built this branch and switched my testing to this go implementation. I can confirm it doesn't have the same issues and i've been able to link >100 repos to a project. There are still unrelated UI issues that mean you have to do this via direct calls to teh rest apis. however this PR seems good and an improved solution overall. (I still had to cusomise it to work for my on-prem use case but that's ok) fwiw if you select api verison 7.0 instead of 7.1 then almost everything works fine on prem also.

wouldd avatar Mar 07 '24 12:03 wouldd

Hi, @mr-ks . I didn't notice that the PR was ready until @wouldd 's comment, seems like the unit test is failing, would you mind taking a look? Feel free to tag me if I did not response in time. Thanks for your contribution.

klesh avatar Mar 08 '24 05:03 klesh

Hi @klesh, unit tests are fixed. I added the plugin in the backend/plugins/table_info_test.gofile, sorry for that. Can you please approve the workflow runs once again?

mr-ks avatar Mar 12 '24 13:03 mr-ks

@mr-ks No worries, I just saw the lint-commit-message didn't pass, it is ok to leave it to fail if it is hard for you to fix. We would accept the PR as long as the code itself is colid.

klesh avatar Mar 13 '24 07:03 klesh

@klesh looks like some prs were merged that had titles not compliant to the conventions. I did a rebase to fix the commit linting step. I hope this is also what you had in mind.

mr-ks avatar Mar 13 '24 20:03 mr-ks

@mr-ks Nice work, actually, this is beyond what I had in mind 👍 Do you think it is ready to be merged? If so, I would need to discuss it with others because we are already in v1.0.0 testing phase.

klesh avatar Mar 14 '24 10:03 klesh

@klesh fwiw I have been testing with this implmentation for the last week or so. I would say it is ready to be merged and it is somewhat suprior to the existing python implementation

wouldd avatar Mar 14 '24 13:03 wouldd