Add command option for output the codeowners directly. #2245

sima-zhu · 2021-11-10T05:45:44Z

The test is in pipeline with the custom tests run.

Here is the test without custom test run:
https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1203228&view=results

azure-sdk · 2021-11-10T19:56:46Z

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

azure-sdk · 2021-11-10T20:58:29Z

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

eng/common/scripts/get-codeowners.ps1

tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs

eng/common/scripts/get-codeowners.ps1

benbp · 2021-11-11T20:51:06Z

I think that adding all this data parsing and vso variable setting logic outside of the direct tool introduces some unnecessary complexity, and will also make it hard for us to detect and make breaking changes to the underlying tool in the future.

I would suggest either:

Move ALL the logic into the codeowners parser tool and support some sort of flag like --set-code-owners-pipeline-variable
If we don't want to set pipeline variables in the C# tool, I would still like to avoid having to parse non-structured output via magic fields in the logs. We could follow a similar approach to what git does with its porcelain vs. plumbing strategy, meaning the tool could take a flag like --porcelain or -o json to specify the output needs to be structured. Then you could have properties like owners and log, and you can parse the json from owners and write-host the log.

With the dotnet tool approach we're moving towards, I think a local or remote script user should be able to use the tools directly and not have to rely too much on wrapper scripts, otherwise we lose some of the benefits of centralized CLI tooling.

weshaggard · 2021-11-11T21:26:35Z

@benbp we should chat more about options and trade-offs here. @sima-zhu is implementing it this was based on guidance from me and exploring the options in different worlds.

The biggest trouble comes when we want to share this code between our devops steps and other tools that need to parse codeowners. If we have the tool set the devops variable then we still have to parse that variable contents and we also have to run it as an independent devops step, which blocks scenarios such as calling it in a loop in another powershell script context. So while I agree with you parsing the output isn't great it essentially is the same as setting the devops variable because that is how those get handled as well (although by DevOps which gives us less flexibility).

benbp · 2021-11-11T21:38:13Z

The biggest trouble comes when we want to share this code between our devops steps and other tools that need to parse codeowners. If we have the tool set the devops variable then we still have to parse that variable contents and we also have to run it as an independent devops step, which blocks scenarios such as calling it in a loop in another powershell script context. So while I agree with you parsing the output isn't great it essentially is the same as setting the devops variable because that is how those get handled as well (although by DevOps which gives us less flexibility).

@weshaggard As you say it's not realistic to do away entirely with wrapper scripts from a pipelines perspective. I guess my issue is more around introducing too much business logic and tight coupling between scripts and the tool output, especially with parsing structured data out of log lines. I think the reverse (printing log lines and extracting info from structured data) is a better pattern to follow for us.

In this scenario we would have minimal script code:

function getCodeOwnersEntryFromCommand() {
  return & "$ToolPath/retrieve-code-owners" `
        --target-directory "$PathToOwners" `
        --root-directory "$WorkingDirectory" `
        -o json
}

function getCodeOwners() { 
  $result = getCodeOwnersEntryFromCommand

  if ($LASTEXITCODE -ne 0) {
    Write-Host $result.Output
    return $null
  }

  $codeOwners = ($result.CodeOwners | ConvertFrom-Json) -join ","
  Write-Host "##vso[task.setvariable variable=$VsoVariable;]$codeOwners"
  return $codeOwners
}

Alternatively, instead of parsing $result.Output you could return the JSON directly as stdout and print all potential issues to stderr, which you can then handle separately and print to the pipeline.

weshaggard · 2021-11-11T23:38:22Z

@sima-zhu, @benbp and I chatted more offline and came to the conclusion that it would be best if we simply have the tool write only the json content to the console in any cases where there is no errors. If there is an error then dump whatever you need to and exit non-zero. That means we can simplify our consumption a little but we will want to go ahead and remove our console logging from the codeowners library. We should also make sure we handle any errors in our conversion from json in case some other output other then the json ends up in the output.

benbp · 2021-11-12T00:08:12Z

but we will want to go ahead and remove our console logging from the codeowners library.

Or just change it to stderr and print it out regardless of exit code (i.e. get rid of the 2>&1 redirect).

weshaggard · 2021-11-12T00:15:12Z

Lets start by removing it and if we find it is useful in some scenario we can figure out how to plumb through a logger.

azure-sdk · 2021-11-12T18:20:25Z

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

azure-sdk · 2021-11-12T19:00:05Z

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

azure-sdk · 2021-11-12T19:08:01Z

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

azure-sdk · 2021-11-12T19:15:36Z

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

eng/common/scripts/get-codeowners.ps1

benbp · 2021-11-12T19:24:15Z

eng/common/scripts/get-codeowners.ps1

+  if (!$codeOwnersJson) {
+    Write-Host "No code owners returned from the path: $CodeOwnerPathExpression"
+    return ""
+  }


Nit: it's not a huge deal here, but as a larger principal, this log statement uses knowledge of how the underlying GetCodeOwnersEntryFromCommand function is implemented (using $CodeOwnerPathExpression), a function which isn't even called by this function. It would be better to log this message from within the GetCodeOwnersEntryFromCommand function since it's specific to its implementation.

I would prefer to delete these lines and throw from the GetCodeOwnersEntryFromCommand function instead, which makes the code simpler.

We fairly get into this condition.
The tool command returns error or json with real context.

if (codeOwnerEntry == null) { Console.Error.WriteLine(String.Format("We cannot find any closest code owners from the target path {0}", targetDirectory)); return 1; } else { var codeOwnerJson = JsonSerializer.Serialize<CodeOwnerEntry>(codeOwnerEntry); Console.WriteLine(codeOwnerJson); return 0; }

The error handling here is for this line

$codeOwnersJson = $codeOwnersString | ConvertFrom-Json

ConvertFrom-Json could go wrong.

My thinking is we could set $ErrorActionPreference to Stop for the script context to handle these cases since ConvertFrom-Json will throw. If the caller has this set in their shell, they'll skip your log statement anyway.

benbp · 2021-11-12T19:27:20Z

tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs

+                }
+                else
+                {
+                    var codeOwnerJson = JsonSerializer.Serialize<CodeOwnerEntry>(codeOwnerEntry);


Do you need to do something like below to get pretty printing of the json, or is it already handled?

Suggested change

var codeOwnerJson = JsonSerializer.Serialize<CodeOwnerEntry>(codeOwnerEntry);

var codeOwnerJson = JsonSerializer.Serialize<CodeOwnerEntry>(codeOwnerEntry, new JsonSerializerOptions { WriteIndented = true });

Nice to have.

azure-sdk · 2021-11-12T19:28:26Z

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

azure-sdk · 2021-11-12T19:29:38Z

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

weshaggard · 2021-11-12T21:02:27Z

eng/common/scripts/get-codeowners.ps1

-  $VsoVariable = "" # target devops output variable
+  [string]$CodeOwnerPathExpression, # Code path to code owners. e.g sdk/core/azure-amqp
+  [string]$ToolVersion = "", # Placeholder. Will update in next PR
+  [string]$ToolPath = "$env:AGENT_TOOLSDIRECTORY", # The place to check the tool existence. Put $(Agent.ToolsDirectory) as default


We might want to actually consider a temp directory for this. That is what @danieljurek is doing for cspell, that would allow for easier running locally as well. We can always change it in DevOps if we want.

I've been using this for cross-platform temp directory lookup.

I've been using something similar:

[Parameter()] [string] $WorkingDirectory = (Join-Path ([System.IO.Path]::GetTempPath()) ([System.IO.Path]::GetRandomFileName())),

This generates a temporary folder name which can then be created later if it doesn't already exist.

eng/common/scripts/get-codeowners.ps1

weshaggard · 2021-11-12T21:04:11Z

eng/common/scripts/get-codeowners.ps1

-  $TargetDirectory, # should be in relative form from root of repo. EG: sdk/servicebus
-  $RootDirectory, # ideally $(Build.SourcesDirectory)
-  $VsoVariable = "" # target devops output variable
+  [string]$CodeOwnerPathExpression, # Code path to code owners. e.g sdk/core/azure-amqp


This isn't the path to code owners this should be called "relativePathInRepoToFindOwners" or something like that.

azure-sdk · 2021-11-12T21:18:18Z

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

weshaggard · 2021-11-12T21:29:47Z

eng/common/scripts/get-codeowners.ps1

  }
+  return & "$ToolPath/retrieve-code-owners" --target-directory "$CodeOwnerPathExpression" --code-owner-file-path "$CodeOwnerFileLocation" 


Does this even work? I ask because I don't see any processing that would handle the --target-directory, I would expect you would need to pass the parameters via position instead of by name.

We have the parameter and I tested locally

Seems like the Dragonfruit dependency is what's providing the magic conversion from the function argument names to the CLI flag syntax?

Dragonfruit magic.

I think this is pretty cool but it might be good to put a code comment above the main method describing that the arguments will be magically handled by the Dragonfruit dependency.

Oh this is interesting and I agree we probably should add a comment there isn't anything to really stop us from removing this dependency and breaking this command line arg parsing.

weshaggard · 2021-11-12T21:36:24Z

eng/common/scripts/get-codeowners.ps1


+  $codeOwners = $codeOwnersJson.Owners -join ","


Move this join inside of the VSOVariable block.

The line is still useful even VSO not set.

weshaggard · 2021-11-12T21:37:00Z

eng/common/scripts/get-codeowners.ps1

+
+InstallRetrieveCodeOwnersTool
+
+$codeOwnerToolOutput = GetCodeOwnersEntryFromCommand


Why not do this in GetCodeOwners function?

Given you aren't really calling these functions more than once you can probably eliminate the functions and just have the code inline.

I agree with moving this code inside GetCodeOwners. Re: eliminating functions, I disagree. I think a lot of our scripts have grown larger over time, and the lack of functions at the start makes our scripts tend to evolve into spaghetti code (New-TestResources.ps1 as the best example of this). I think if we try to keep all logic in functions except for a single entrypoint function call at the end of the script, it's easier to modify the code AND easier to test because you can dot source individual functions and execute them locally (provided you filter on invocation name).

Yes testing is definitely a great reason to have these functions.

weshaggard · 2021-11-12T21:37:23Z

eng/common/scripts/get-codeowners.ps1

+
+$codeOwnerToolOutput = GetCodeOwnersEntryFromCommand
+# Failed at the command of fetching code owners.
+if ($LASTEXITCODE -ne 0) {
  return ""


We should probably return an empty list.

Why don't we return string with ","
I feel like string is much easy to deliver between yaml and scripts. It is also easy to parse.

I think to keep both the object format and the yaml<-->script format, this could be a JSON stringified empty list, i.e. "[]" or @() | ConvertTo-Json.

weshaggard · 2021-11-12T21:37:40Z

eng/common/scripts/get-codeowners.ps1

  return ""
 }
+GetCodeOwners $codeOwnerToolOutput


Probably should be a return statement so it is clear.

tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs

check-enforcer-staging · 2021-11-18T19:00:31Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

tools/code-owners-parser/custom-tests.yml

eng/pipelines/templates/stages/archetype-sdk-tool-dotnet.yml

tools/code-owners-parser/custom-tests.yml

eng/pipelines/templates/stages/archetype-sdk-tool-dotnet.yml

sima-zhu requested a review from a team as a code owner November 10, 2021 05:45

sima-zhu requested review from weshaggard and removed request for a team November 10, 2021 17:46

sima-zhu force-pushed the output_psmodule branch from 8bb9278 to eeab17f Compare November 10, 2021 19:52

weshaggard reviewed Nov 10, 2021

View reviewed changes

eng/common/scripts/get-codeowners.ps1 Outdated Show resolved Hide resolved

weshaggard reviewed Nov 10, 2021

View reviewed changes

tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs Outdated Show resolved Hide resolved

weshaggard reviewed Nov 10, 2021

View reviewed changes

benbp reviewed Nov 11, 2021

View reviewed changes

eng/common/scripts/get-codeowners.ps1 Outdated Show resolved Hide resolved

benbp reviewed Nov 11, 2021

View reviewed changes

eng/common/scripts/get-codeowners.ps1 Outdated Show resolved Hide resolved

sima-zhu requested review from benbp and weshaggard November 12, 2021 19:12

benbp reviewed Nov 12, 2021

View reviewed changes

eng/common/scripts/get-codeowners.ps1 Outdated Show resolved Hide resolved

benbp reviewed Nov 12, 2021

View reviewed changes

eng/common/scripts/get-codeowners.ps1 Outdated Show resolved Hide resolved

benbp reviewed Nov 12, 2021

View reviewed changes

eng/common/scripts/get-codeowners.ps1 Outdated Show resolved Hide resolved

benbp reviewed Nov 12, 2021

View reviewed changes

weshaggard reviewed Nov 12, 2021

View reviewed changes

eng/common/scripts/get-codeowners.ps1 Outdated Show resolved Hide resolved

weshaggard reviewed Nov 12, 2021

View reviewed changes

tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs Outdated Show resolved Hide resolved

kurtzeborn mentioned this pull request Nov 15, 2021

Vcpkg comment tag failed Azure/azure-sdk-for-cpp#3061

Closed

sima-zhu force-pushed the output_psmodule branch from dadad0f to f42263d Compare November 18, 2021 18:54

sima-zhu force-pushed the output_psmodule branch from fb09d44 to 196995a Compare November 18, 2021 21:52

sima-zhu requested review from benbp and weshaggard November 18, 2021 22:20

benbp reviewed Nov 18, 2021

View reviewed changes

tools/code-owners-parser/custom-tests.yml Outdated Show resolved Hide resolved

benbp approved these changes Nov 18, 2021

View reviewed changes

weshaggard reviewed Nov 19, 2021

View reviewed changes

eng/pipelines/templates/stages/archetype-sdk-tool-dotnet.yml Outdated Show resolved Hide resolved

weshaggard reviewed Nov 19, 2021

View reviewed changes

tools/code-owners-parser/custom-tests.yml Outdated Show resolved Hide resolved

Added test post steps

9d8bcfe

sima-zhu force-pushed the output_psmodule branch from 62a8bab to 9d8bcfe Compare November 19, 2021 00:41

sima-zhu requested a review from weshaggard November 19, 2021 00:45

Added missing files

2d863ac

weshaggard approved these changes Nov 19, 2021

View reviewed changes

weshaggard reviewed Nov 19, 2021

View reviewed changes

eng/pipelines/templates/stages/archetype-sdk-tool-dotnet.yml Outdated Show resolved Hide resolved

move the post step one step upper

5414f4d

sima-zhu merged commit 7724333 into Azure:main Nov 19, 2021

sima-zhu deleted the output_psmodule branch November 19, 2021 00:59

weshaggard mentioned this pull request Nov 19, 2021

Install and Run code owner tools in get-codeowner.ps1 #2322

Merged

	var codeOwnerJson = JsonSerializer.Serialize<CodeOwnerEntry>(codeOwnerEntry);
	var codeOwnerJson = JsonSerializer.Serialize<CodeOwnerEntry>(codeOwnerEntry, new JsonSerializerOptions { WriteIndented = true });

		}
		return & "$ToolPath/retrieve-code-owners" --target-directory "$CodeOwnerPathExpression" --code-owner-file-path "$CodeOwnerFileLocation"


		InstallRetrieveCodeOwnersTool

		$codeOwnerToolOutput = GetCodeOwnersEntryFromCommand

Add command option for output the codeowners directly. #2245

Add command option for output the codeowners directly. #2245

Conversation

sima-zhu commented Nov 10, 2021 • edited Loading

azure-sdk commented Nov 10, 2021

azure-sdk commented Nov 10, 2021

benbp commented Nov 11, 2021 • edited Loading

weshaggard commented Nov 11, 2021

benbp commented Nov 11, 2021 • edited Loading

weshaggard commented Nov 11, 2021

benbp commented Nov 12, 2021

weshaggard commented Nov 12, 2021

azure-sdk commented Nov 12, 2021

azure-sdk commented Nov 12, 2021

azure-sdk commented Nov 12, 2021

azure-sdk commented Nov 12, 2021

Choose a reason for hiding this comment

sima-zhu Nov 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbp Nov 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azure-sdk commented Nov 12, 2021

azure-sdk commented Nov 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azure-sdk commented Nov 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbp Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

check-enforcer-staging bot commented Nov 18, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

sima-zhu commented Nov 10, 2021 •

edited

Loading

benbp commented Nov 11, 2021 •

edited

Loading

benbp commented Nov 11, 2021 •

edited

Loading

sima-zhu Nov 12, 2021 •

edited

Loading

benbp Nov 12, 2021 •

edited

Loading

benbp Nov 18, 2021 •

edited

Loading