search

MethodsAndPractices / vsteam

PowerShell module for accessing Azure DevOps Services and Azure DevOps Server (formerly VSTS or TFS)
https://methodsandpractices.github.io/vsteam-docs/
MIT License
445 stars 155 forks source link

Items #299, #300, #301 #302 and #304 #303

Closed jhoneill closed 4 years ago

jhoneill commented 4 years ago

PR Summary

PR Checklist

SebastianSchuetze commented 4 years ago

@jhoneill thanks for the PR. In the future could you please not have so many changes in one PR. It does not speed up a review. It is quite the opposite. One PR for each issue and usually each issue should try to handle one use case or topic.

Would it be possible for you to split them up? I know it is some more work for you, but it reduces work/review packages and makes it easier to understand.

jhoneill commented 4 years ago

I can try to do that in future. I try to keep to one commit per area. I'm about to send you ~20 new commands (Add-Process, Add-WorkItemItemType, and stuff to create fields and layout workitem forms) and it is fairly difficult to break those into small pieces . Because the parts depend on each other if I send you (for example) may update for processes as one PR, and the update for work item types as second, PR #2 (a) contain all of PR #1 and (b) Needs to come from a new branch (if I commit into the branch I sent in PR1 that github adds them to the open request i.e. the pull by branch not by commit).

If you like I can roll back to Reduce scope of default -Project parameter to *-Vsteam* commands make a branch and a PR then to
Change formats.json to include all .ps1xml files in formats and do the same and then to Allow Get-VSteamWorkitemType to use cached process info and to expand then and do the same for the last 3. Does 6 PRs with each one depending on those before it help you ? My hunch is not. Because if you change something I did in the first when it is merged, it then needs to be changed in the other five. So I'd assumed that one PR with work in multiple commits would show what depended on what else. If that's not working for you , let me know and I'll try to adapt. Cheers J

jhoneill commented 4 years ago

Do let me know what your preference is. Including help, which means commands get 3 files, and formatting information what's in this PR plus the next block of work is ~ 120-130 new or changed files. They divide into ~19 sections (got a spreadsheet with that in front of me). I can withdraw this one this one and make 19 branches and 19 PRs , or keep this one and send you the additions as one or two PRs with a list of what's different and why. The latter is less work for me, but I don't mind doing that work if it helps you.

SebastianSchuetze commented 4 years ago

Thanks for your tips and effort. I have not much time currently as I am doing this in my free time. This is not part of my daily job. I will think about it as soon as I have some time to think about it.

jhoneill commented 4 years ago

No worries. This came about as a by product of my day job :-) But getting it fit to share is/was my own time.

DarqueWarrior commented 4 years ago

You can't do this.

Change formats.json and types.json to list "*" instead of individual files. This won't work for classes because they must be loaded in the correct sequence

The order of formats is important. The tables have to be listed first. If you do a * it will be in alphabet order and the list formats will be first which is not what I want. The Readme.md in the folder explains why this can't be done.

I also think your branch must be way behind because types in master has already been changed to a * format.

Please make sure you have the latest changes in your branch.

In the future please make smaller PRs so we can see each change and not have to reject the entire PR.

DarqueWarrior commented 4 years ago

This change has already been made

Change scope of default -Project parameter to apply only to -Vsteam commands (#297 - impacts _clearEnvironmentVariables, get-VSTeamInfo *-VSTeamDefaultProject)

DarqueWarrior commented 4 years ago

Already fixed

Change _callAPI to trap being called when no logon/account is set.

DarqueWarrior commented 4 years ago

Change Invoke-vsteamRequest. (#302) Switch order of parameters "area" and "resource" and give them completers. Add a switch '-ExpandValue' Example: Type Invoke-vs [Tab] [-] [Tab] "-area" completes (first on the url path) [space] [Tab] "build" etc complete; for "WIT" -Resource [Tab] will suggest "workitemtypecategories" etc. add -value or -ExpandValue and instead of Count=15; Value=[Array], the array is returned

You don't have to change the order. This will be in next version.

jhoneill commented 4 years ago

Change formats.json and types.json to list "*" instead of individual files.

The order of formats is important. .... The Readme.md in the folder explains why this can't be done.

My apologies. I missed that and was trying to take a short cut. I will revert to the approved way.

I also think your branch must be way behind ...

Please make sure you have the latest changes in your branch.

I had sync'd up to date I put the the PR in. It looks more things have gone in since. Last that I saw, you were doing the change of scope for the default parameter but it hadn't gone in, and I did the catch for no active logon/account raised it as an issue, and did the PR in a very short space of time, and we seem to have duplicated each other there.

Change Invoke-vsteamRequest. (#302) Switch order of parameters "area" and "resource" and give them completers. Add a switch '-ExpandValue'

You don't have to change the order. This will be in next version. OK I'll back this out.

In the future please make smaller PRs so we can see each change and not have to reject the entire PR.

Yes. I'm sorry for sending such large lump. For replacing dynamic parameters with the completers and validator classes that was unavoidable. It is far to easy for me to lapse into "While I am doing X I will just fix Y", which feels like being helpful, but isn't always a good way to deliver help.

I have a big block of work which I am waiting to send you. We got asked to configure some ProcessTemplates in other words, "Add "Scrum 2" as our version of scrum, add work item types to it, set up layouts, fields, statuses, boards for the those work item types. Make it almost the same as this other DevOps Org, but change this to that ... etc.".
We ended up doing "Process Template as code" :-) and I wrote these new functions

Add-VSTeamField
Add-VSTeamPickList
Add-VSTeamProcess
Add-VSTeamProcessBehavior
Add-VsteamWorkItemControl
Add-VsteamWorkItemField
Add-VsteamWorkItemPage
Add-VsteamWorkItemPageGroup
Add-VsteamWorkItemState
Add-VSTeamWorkItemType
Get-VSTeamBuildTimeline
Get-VSTeamField
Get-VSTeamPickList
Get-VSTeamProcessBehavior
Get-VSTeamWorkItemBehavior
Get-VsteamWorkItemField
Get-VsteamWorkItemPage
Get-VsteamWorkItemState
Hide-VsteamWorkItemState
Remove-VsteamWorkItemState
Remove-VSTeamWorkItemType
Set-VSTeamPickList
Set-VSTeamProcess
Set-VSTeamWorkItemBehavior
Set-VSTeamWorkItemType
Show-VsteamWorkItemState

They leverage the completers / validators I have already given you and add some more, and in doing the new work, I found the existing completers should wrap some values in quotes and the validators should not reject null or empty (if a parameter is mandatory Null/empty needs to be explicitly allowed and if not it needs to be forbidden and that's not the validator's job).

I have also moved the responsibility for managing the object caches into their classes so they now have Update, GetCurrent (which will update if the cache is empty or stale) and Invalidate to force the next cycle to update. Where you had a method for determining "stale" I have left it, and the properties remain exposed for anything which relied on directly accessing them.

Things like Get-Process now update the cache if they do a "fetch all" using that and I made another change to get process : there are two different places you can call for process information and I wanted the richer one as a pre-req for the new commands.

What this PR was trying to be was "the changes to the already released things which are pre-requisites for the new functions above". But I seem to have made more problems than benefits. So I propose to

  1. Close this this PR.
  2. Sync what I have with your master branch. Currently I have "official" which is your master, "Master" which is this PR and a feature branch with all the extra changes in it. Some re-arrangement of my branches is going to be needed.
  3. Back out my changes to the parameter order of Invoke, and to how things are built
  4. Send you a new PR with only the updates to the completers/ associated cache classes, and the new completers for Invoke-, you are changing Invoke so you can add the completers or not in doing that.
  5. File a new issue saying "Build the classes into the PSM1" with the changes required. I will leave you to change build.ps1
  6. Send you my changes to Get-VSTeamProcess and to the Process Object as a separate PR - it depends on the completer updates, and the long list above depends on it.
  7. Send you my changes to Get-VSTWorkItemType again the same dependencies.
  8. Send you the new commands. What would be the best way to send the changes ? . There are two dozen functions each with a PS1 and 2 .MD files, plus some format files, plus some additional classes so the total is getting up near 100 files, and I think that gives Sebastian nightmares :-) I can group as
    • Processes
    • WorkItem types & their states
    • "Behaviors" (boards) for processes and work item types
    • Fields and picklists
    • Associate fields with WorkItem-types and laying them out

I think that ends up as 8 PRs. Does that work for you ? Best etc. J

SebastianSchuetze commented 4 years ago

I think redoing the PR would help us much better.

8. Send you the new commands. What would be the best way to send the changes ? . There are two dozen functions each with a PS1 and 2 .MD files, plus some format files, plus some additional classes so the total is getting up near 100 files, and I think that gives Sebastian nightmares :-) I can group as

The best way to do it I think if keeping related files together. Look at the PR #286 which was grouping cmdlets for classification nodes. You could do the same for the cmdlets.

E.g. include in the same PR

make sure they follow the same style as in my PR earlier.

If you have changes that apply to multiple cmdlets then they should come early included but they should not be dependant from anything. Meaning they should work without the changes coming later that are dependant.

Also make sure first that our pipeline is going through with all environments. We do not usually start to review before the pipeline is not succeeding.

And thanks for your understanding. Because most of the problem in open source is:

DarqueWarrior commented 4 years ago

I already wrote the completer for Invoke last night and is already in master.

DarqueWarrior commented 4 years ago

I like the idea of grouping PRs around a noun "WorkItemState" all the verbs, tests and doc for that noun can be a single PR.

DarqueWarrior commented 4 years ago

Finally there is no need to apologize for helping. We really appreciate your support and understanding.

SebastianSchuetze commented 4 years ago

Fully agree with @DarqueWarrior. Help is much appreciated since most of it is a personal effort and the drive to improve things.

jhoneill commented 4 years ago

Thanks both, I'll stop saying sorry. It's not that I'm talking down what I've done, just aware that I could have delivered it a way that's more convenient for you. And sometimes explaining what one has done with only text to go on, it can look like "I AM RIGHT BECAUSE" so I've have taken to saying "Whoops, I ..." "My bad, it was trying to..." "Sorry, I thought ..." to save accidentally causing an argument. :-)