-
-
Notifications
You must be signed in to change notification settings - Fork 933
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Integrate with Azure Devops #2388
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello there BalighMehrez! 👋
Thank you and congrats 🎉 for opening your first PR on this project! ✨ 💖
We will try to review it soon!
Thank you very much!! I try to have a look tomorrow! |
There seem to be some minor linting errors:
|
export const AZUREDEVOPS_POLL_INTERVAL = 10 * 60 * 1000; | ||
export const AZUREDEVOPS_INITIAL_POLL_DELAY = 8 * 1000; | ||
|
||
export const AZUREDEVOPS_API_BASE_URL = 'https://dev.azure.com'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thought of this integration when discussing in my team, awesome that you are on it! Will definitely try this out. One request: can you please make this URL configurable? My employer uses on-premise Azure DevOps Server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi thanks a lot for working on it! 🎉 I am personally curious to test this out. I've left some comments, where I see potential problems (especially with the self-hosted/on-premise Azure DevOps Server) or ideas for improvement.
); | ||
} | ||
|
||
wiql$(cfg: AzuredevopsCfg, query: string): Observable<AzuredevopsWIQLSearchResult> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this abbreviation about? Maybe use a non-abbreviated name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIQL is a query language used to query work items form Azure DevOps APIs, it stands for (Work Items Query Language).
getById$(issueId: number, cfg: AzuredevopsCfg): Observable<AzuredevopsIssue> { | ||
return this._sendRequest$( | ||
{ | ||
url: `${BASE}/${cfg.organization}/${cfg.project}/_apis/wit/workitems/${issueId}?fields=${WIT_FIELDS_STR}&api-version=7.1-preview.3`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api-version=7.1-preview.3
-> maybe use more stable API version? I.e. at least not a preview build?
This table here shows the latest stable is 6.0, which is Azure DevOps Server 2020 (build 18.170.30525.1), your version would be 7.1, which is a good development target, but the integration should be possible with at least some somewhat recent version.
Also it would be good to note somewhere, with which Azure DevOps version this is compatible/which does it require?
See also: API docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point,
this should be configurable for compatibility with on-premise versions, as some users still work with older versions as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, depends on how the API may have changed in previous versions (i.e. which is the minimal API version your code works with). But if it is configurable, then people could just "try it out".
const owner = cfg.filterUsername; | ||
return this.wiql$( | ||
cfg, | ||
` Select [System.Id], [System.Title], [System.State] From WorkItems Where [State] <> 'Closed' AND [State] <> 'Removed' AND [System.AssignedTo] contains '${owner}' order by [Microsoft.VSTS.Common.Priority] asc, [System.CreatedDate] desc `, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This query should potentially be configurable, should not it? At least for the Jira integration, their query system is configurable in SuperProducitivity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One use case I have in mind is that one may want to filter the items by their Epic, because one may use one epic per (sub)project.
export class AzuredevopsApiService { | ||
constructor(private _snackService: SnackService, private _http: HttpClient) {} | ||
|
||
getById$(issueId: number, cfg: AzuredevopsCfg): Observable<AzuredevopsIssue> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO abbreviations such as cfg
for config
should be discouraged, as it decreases readability, but that is a thing the maintainer should decide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with you, but I wanted it to be much closer to current integrations, so I have just used same abbreviations and naming conventions I have found.
"WRITE_A_COMMENT": "Write a comment" | ||
}, | ||
"S": { | ||
"ERR_UNKNOWN": "Azure Devops: Unknown error {{statusCode}} {{errorMsg}}. Api Rate limit exceeded?" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"ERR_UNKNOWN": "Azure Devops: Unknown error {{statusCode}} {{errorMsg}}. Api Rate limit exceeded?" | |
"ERR_UNKNOWN": "Azure Devops: Unknown error {{statusCode}} {{errorMsg}}. API rate limit exceeded?" |
spelling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"ERR_UNKNOWN": "Azure Devops: Unknown error {{statusCode}} {{errorMsg}}. Api Rate limit exceeded?" | |
"ERR_UNKNOWN": "Azure DevOps: Unknown error {{statusCode}} {{errorMsg}}. API rate limit exceeded?" |
Also AFAIK/IIRC that is the official spelling of the service.
export const mapAzuredevopsReducedIssueFromWIQL = ( | ||
workItem: any, | ||
): AzuredevopsIssueReduced => { | ||
console.log(workItem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just there for debugging and should be removed in prod code then, should not it?
@@ -0,0 +1,56 @@ | |||
@import 'src/variables'; | |||
|
|||
// table styled by ./src/styles/components/issue-table.scss |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
random thought: would not an @import ./src/styles/components/issue-table.scss
be possible instead of coping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I am not familiar with SCSS. I have copied from another issue module and replaced names 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well then you could just try that out, could not you? That @import
is actually part of CSS (now), and it works quite simple: https://developer.mozilla.org/en-US/docs/Web/CSS/@import
return { | ||
title: this._formatIssueTitle(issue.id, issue.title), | ||
issueWasUpdated: false, | ||
// NOTE: we use Date.now() instead to because updated does not account for comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// NOTE: we use Date.now() instead to because updated does not account for comments | |
// NOTE: we use Date.now() instead, because updated does not account for comments |
grammar
|
||
issueLink$(issueId: number, projectId: string): Observable<string> { | ||
return this._getCfgOnce$(projectId).pipe( | ||
map((cfg) => `https://azuredevops.com/${cfg.organization}/issues/${issueId}`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs to be configurable/use the configured base url for hosted/on-premise Azure DevOps Server instances, otherwise the link is broken... 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I am unsure what is different about our on-premise Azure DevOps instance (maybe the template?), but we have a different URL format and the above one does not work. It's https://{host}/tfs/{collection|organisation}/{project}/_workitems/edit/{issueId}
.
Collections: https://learn.microsoft.com/en-us/azure/devops/server/admin/manage-project-collections?view=azure-devops-2022
Projects: https://learn.microsoft.com/en-us/azure/devops/organizations/projects/create-project?view=azure-devops-2022&tabs=browser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is Azure DevOps Server 2020 Update 1.1. Also tried the URL on Azure DevOps Server 2022 (AzureDevOpsServer_20221122.1) and it does not work either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this link it totally wrong, will not work in hosted either.
} | ||
private _b64EncodeUnicode(str: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
private _b64EncodeUnicode(str: string): string { | |
} | |
private _b64EncodeUnicode(str: string): string { |
Whats the status on this? I would really love to see a DevOps Integration. |
The linting stuff needs to be fixed. Apart from that I am open to merge this. Unfortunately I can't test the feature myself, so help with that would be welcome! |
@johannesjo I'd like to fix the linting errors, but the job's log is expired. Could you please rerun it so I can see whats wrong? |
@LukasKlepper looks like I can't :/ But you can also run all the linting/unit testing stuff locally via:
If this doesn't work for you, you can retrigger the build by submitting a new commit. |
Hi @johannesjo, before I've looked into the linting errors I've checked the integration with devops myself and saw some things which may need to improve before matching the quality of a master pr.
The linting errors seem quite easy to fix.
|
Attention: This depends on the template you use/have in ADS. You have several different ones and AFAIK you can also customize them. In my experience we log to Tasks, mostly. e.g. and the parent of that can e.g. be called Product Backlog Item (PBI) in the Scrum process. |
@rklec Yeah, it depends mostly on the organization guidelines/settings. Thats why it should only show the type of the item so the user can decide whats important for him. |
Keeping a watchful eye, would be a great addition to my workflow to have this! |
Second this, would be amazing to have! |
This PR has not received any updates in 90 days. Please comment, if this still relevant! |
Very relevant. |
What's the status on this? Any chance of getting it merged? |
I am unable to test this myself, so if someone were to test this and report how it is working back here, I am all for merging this. |
Note you can AFAIK create a free(?) account on https://dev.azure.com/ and test it. Though this obviously only tests the hosted/SaaS solution. |
I use Azure DevOps daily, and this feature would be very useful! |
That would be great! Let me know if you have any questions! |
@tapaolo I can help testing, unfortunely currently don't have experience with angular and thus not the time for learning & implementation. |
Are there any updates on this? |
Frustratingly the PR is super behind main now. Seeing what I can do to test. Basic functionality is there: @johannesjo
@BalighMehrez Would you be able to update from master? Otherwise I guess I could fork it and try to merge master in myself? |
Update: I have been running this PR locally after updating from main. Haven't gotten around to fixing the linting yet, but it's been working well for me. Pulls stuff from Azure effortlessly. I've been using it for a couple weeks now. |
@Stan-Stani let me know if you need any help with it! |
Description
Integration with Azure DevOps boards.
Issues Resolved
#356
Check List