Skip to content
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

support for task groups (and a couple other super minor tweaks) #199

Closed

Conversation

quintessential5
Copy link
Contributor

PR Summary

  • support for task groups
  • a typo fix
  • a minor tweak to my last PR: I didn't _applyTypesToVarGroup on the object returned.
  • Last PR (support for name when getting var groups) I didn't specifically verify that I could generate the help files. I tried to this time, but received a number of errors. I'm OK to not troubleshoot it further if it generates fine for you, as I don't have any further PR's in mind as of now.

PR Checklist

@quintessential5
Copy link
Contributor Author

quintessential5 commented Sep 12, 2019

@DarqueWarrior, I'm guessing you already saw the failed job, but figured I'd mention you just in case.

I'm at a loss as to what the issue is. I see this pipeline worked for you on 9/6 (https://loecda.visualstudio.com/Team%20Module/_build/results?buildId=1635&view=results) and the last change was 9/5 (https://github.com/DarqueWarrior/vsteam/commits/master/azure-pipelines.yml), so I'm not sure why it would magically fail now.

Also, it looks like the NuGet and PS versions used in the failing task is the same for your green run (https://loecda.visualstudio.com/3e857acd-880f-4056-a46b-1de672ca55cc/_apis/build/builds/1635/logs/51) as for my red run (https://loecda.visualstudio.com/3e857acd-880f-4056-a46b-1de672ca55cc/_apis/build/builds/1637/logs/42), so no joy there.

I can only think some under the hood AzD change?

@DarqueWarrior
Copy link
Collaborator

Going to look at it now

Copy link
Collaborator

@DarqueWarrior DarqueWarrior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure in the help files you have YAML section for all the parameters.
Add description to your examples.
Don't forget your SupportShouldProcess for Remove and Updates and add Force parameters.

.docs/Add-VSTeamTaskGroup.md Show resolved Hide resolved
.docs/Add-VSTeamTaskGroup.md Show resolved Hide resolved
.docs/Add-VSTeamTaskGroup.md Show resolved Hide resolved
.docs/Get-VSTeamTaskGroup.md Show resolved Hide resolved
.docs/Get-VSTeamTaskGroup.md Show resolved Hide resolved
Source/Classes/VSTeamVersions.ps1 Show resolved Hide resolved
@@ -0,0 +1,27 @@
function Remove-VSTeamTaskGroup {
[CmdletBinding()]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add SupportsShouldProcess = $true and a ConfirmImpact value. You can review Remove-VSTeamBuildTag as an example.

@@ -0,0 +1,31 @@
function Update-VSTeamTaskGroup {
[CmdletBinding()]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add SupportsShouldProcess = $true and a ConfirmImpact value. You can review Update-VSTeamBuildDefinition for an example.

Source/Public/Add-VSTeamTaskGroup.ps1 Show resolved Hide resolved
$resp = _callAPI -Method Put -ProjectName $ProjectName -Area distributedtask -Resource taskgroups -Version $([VSTeamVersions]::TaskGroups) -Body $Body -ContentType 'application/json' -Id $Id
}

return $resp
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you missing a call to _applyTypesToVariableGroup?

@quintessential5
Copy link
Contributor Author

Working on the requested changes.

Should I worry if .docs\gen-help.ps1 has a ton of errors? If it does, does that mean I'm doing something incorrectly? Maybe it's only meant to be run in the AzD build process?

@DarqueWarrior
Copy link
Collaborator

I have pulled the latest changes and looking around now. Thanks again for the support.

@DarqueWarrior
Copy link
Collaborator

DarqueWarrior commented Oct 3, 2019

Working on the requested changes.

Should I worry if .docs\gen-help.ps1 has a ton of errors? If it does, does that mean I'm doing something incorrectly? Maybe it's only meant to be run in the AzD build process?

I just ran it on your master branch and everything worked as expected.

@quintessential5
Copy link
Contributor Author

Working on the requested changes.
Should I worry if .docs\gen-help.ps1 has a ton of errors? If it does, does that mean I'm doing something incorrectly? Maybe it's only meant to be run in the AzD build process?

I just ran it on your master branch and everything worked as expected.

Weird. In any case, I'm sure you'd let me know if the documentation isn't as you were expecting. Thanks for checking it out!

@quintessential5
Copy link
Contributor Author

quintessential5 commented Oct 3, 2019

Sorry, I'm realizing that I'm a bit confused as to where we are. I see that you've closed this request rather than merged. I guess you're just planning on looking into the TFS 2017 support, as you mentioned above. You also wrote, "If I determine it is not supported in TFS 2017 we will leave "" and throw when someone tries to use in an unsupported version." I guess you mean in that case, that VSTeam won't support task groups? I guess, in any case, I'll hold off on implementing Force / ShouldProcess for the Remove and Update functions.

@quintessential5
Copy link
Contributor Author

Sorry, I'm realizing that I'm a bit confused as to where we are. I see that you've closed this request rather than merged. I guess you're just planning on looking into the TFS 2017 support, as you mentioned above. You also wrote, "If I determine it is not supported in TFS 2017 we will leave "" and throw when someone tries to use in an unsupported version." I guess you mean in that case, that VSTeam won't support task groups? I guess, in any case, I'll hold off on implementing Force / ShouldProcess for the Remove and Update functions.

Correction: looks like I had already implemented Force / ShouldProcess. I'm now just fixing some unit tests in my environment-specific wrapper module to make sure my implementation is OK. I guess I should make a new pull request for that?

@quintessential5
Copy link
Contributor Author

@DarqueWarrior, no rush at all. Just making sure you saw the above comments so we don't duplicate effort on Force / ShouldProcess. I'm guessing maybe GitHub doesn't notify you on my comments to my own PR?

@DarqueWarrior
Copy link
Collaborator

Can you make sure I have commit permissions to this PR.

@quintessential5
Copy link
Contributor Author

Can you make sure I have commit permissions to this PR.

Googling how to set that up...

@quintessential5
Copy link
Contributor Author

Can you make sure I have commit permissions to this PR.

Googling how to set that up...

Done.

@DarqueWarrior
Copy link
Collaborator

I made several changes and updated the formats for you. I think I could be able to merge the changes when you re-open it.

@quintessential5
Copy link
Contributor Author

Super weird: I've clicked the checkbox below multiple times, and it appears to take effect, but if I refresh the page, it reverts to being unchecked:
image

@quintessential5
Copy link
Contributor Author

I also don't see an obvious way to re-open:

image

Maybe since you closed it, I don't have permission to re-open? Maybe once it's re-opened the permission to let you commit will stick?

@quintessential5
Copy link
Contributor Author

Yeah, I see that there should be a "Reopen pull request" button:

image

... that I don't have:

image

... so, I'm guessing you have the power :)

@quintessential5
Copy link
Contributor Author

@DarqueWarrior, no rush at all, just for clarity: just let me know if you can't re-open this PR (which I'm hoping will let us fix your ability to commit to it). If you can't re-open, I'll just make a new PR.

@quintessential5 quintessential5 mentioned this pull request Oct 21, 2019
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants