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

Tilee/add release scripts #206

Merged
merged 8 commits into from Apr 13, 2018

Conversation

Projects
None yet
3 participants
@MS-TimothyMothra
Member

MS-TimothyMothra commented Apr 13, 2018

storing scripts for our release process in a central repo

@d3r3kk

d3r3kk approved these changes Apr 13, 2018

Great work, thanks so much for getting us on the road to CI/CD automation awesomeness.

A few nits from a PowerShell dude but if it is working feel free to keep & change what you wish. We can always slowly improve things as we go.

@@ -0,0 +1,194 @@
Param(

This comment has been minimized.

@d3r3kk

d3r3kk Apr 13, 2018

Contributor

It is generally accepted practice to add a header to your PowerShell scripts. A great primer is installed and you can view it by running:

get-help about_Comment_Based_Help

...from the powershell command line.

This comment has been minimized.

@d3r3kk

d3r3kk Apr 13, 2018

Contributor

NIT: Powershell scripts are generally named in the pattern VERB-ADJECTIVE-NOUN. See powershell cmdlet Get-Verb for a list of expected verbs.

This comment has been minimized.

@MS-TimothyMothra

MS-TimothyMothra Apr 13, 2018

Member

oh, I didn't know the same naming convention applied to files.

$sourcePath,
[Parameter(Mandatory=$false,HelpMessage="Path to save metadata:")]
[string]
$outPath

This comment has been minimized.

@d3r3kk

d3r3kk Apr 13, 2018

Contributor

If this is a non-mandatory parameter, feel free to set it to something (unless you want to set a null-value explicitly later as part of your script).

$outPath = ".\output\"

This comment has been minimized.

@MS-TimothyMothra

MS-TimothyMothra Apr 13, 2018

Member

good catch. it's supposed to be mandatory.
everything was set to $false when i was testing locally.
Fixed!

}
Function Get-GitChangeset() {
# if running localy, use git command. for VSTS, probably better to use Build.SourceVersion

This comment has been minimized.

@d3r3kk

d3r3kk Apr 13, 2018

Contributor

Using comments atop the functions are good.

# Get-GitChangeset
# if running locally, ...
function Get-GitChangeset() {
# get everything up to word "-build" (ex: "1.2.3-beta1-build1234 returns: "1.2.3-beta1")
# get everything (ex: "1.2.3-beta1 returns: "1.2.3-beta1")
# get everything (ex: "1.2.3 returns: "1.2.3")
$splitVersion = $version.split('-')

This comment has been minimized.

@d3r3kk

d3r3kk Apr 13, 2018

Contributor

PowerShell-ify NIT:
`$splitVersion = $version -split '-'

$this.NuspecVersion = $nuspecVersion
$this.DllVersion = $dllVersion
$mygetBasePath = "https://www.myget.org/feed/applicationinsights/package/nuget/{0}/{1}"

This comment has been minimized.

@d3r3kk

d3r3kk Apr 13, 2018

Contributor

PowerShell NIT:
Just put the variables right into the string:

$mygetBasePath = "https://.../nuget/$name/$nuspecVersion"

If you are using properties of something within the string, just enclose it in $() like so:

$mygetBasePath = "https://.../nuget/$($name)/$($nuspecVersion)"
if($readFile) {
if($saveLines) {
if($_ -like "##*") {

This comment has been minimized.

@d3r3kk

d3r3kk Apr 13, 2018

Contributor

PowerShell NIT:
I believe the current accepted practice is to prefer $PSItem over $_.

This comment has been minimized.

@d3r3kk

d3r3kk Apr 13, 2018

Contributor

Also, for readability it is sometimes nice to switch to a local variable within loops.

$listItems | ForEach-Object {
  $anItem = $PSItem
  ...
return $sb.ToString()
}
Function Save-ToXml([string]$outPath, [ReleaseInfo]$object) {

This comment has been minimized.

@d3r3kk

d3r3kk Apr 13, 2018

Contributor

What happens with the [ReleaseInfo] object if you simply use the ConvertTo-Xml cmdlet that is built into PowerShell? Perhaps you can skip this?

This comment has been minimized.

@MS-TimothyMothra

MS-TimothyMothra Apr 13, 2018

Member

Oh? I didn't find this command when I was searching. I'll test it.

This comment has been minimized.

@MS-TimothyMothra

MS-TimothyMothra Apr 13, 2018

Member

this does some weird formatting and would create a lot of new work to change the downstream scripts.
I'm going to skip this for now.

$xmlWriter.Close()
}
# 1) GET META DATA FROM ARTIFACTS

This comment has been minimized.

@d3r3kk

d3r3kk Apr 13, 2018

Contributor

I love this explicit call-out of steps.

$emailBody = $sb.ToString()
Write-Output $emailBody

This comment has been minimized.

@d3r3kk

d3r3kk Apr 13, 2018

Contributor

Maybe convert this script to output the email body/etc... to a file?

This comment has been minimized.

@MS-TimothyMothra

MS-TimothyMothra Apr 13, 2018

Member

Lets discuss this more in a couple weeks. I'm still optimistic I can get email to work.

@@ -0,0 +1,85 @@
Param(

This comment has been minimized.

@d3r3kk

d3r3kk Apr 13, 2018

Contributor

General comments I've given above apply to this file as well.

@MS-TimothyMothra MS-TimothyMothra merged commit 6735f5d into master Apr 13, 2018

1 check passed

license/cla All CLA requirements met.
Details

@MS-TimothyMothra MS-TimothyMothra deleted the tilee/add_release_scripts branch Apr 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment