Adding migration of GitHub Secret Scanning Alerts

[peter-murray]

• [x] Did you write/update appropriate tests
• [ ] Release notes updated (if appropriate)
• [x] Appropriate logging output
• [x] Issue linked
• [ ] Docs updated (or issue created)

This is the first pass on the requirements to migrate secret scanning remediations between two repositories, provides the functionality for #536.

This is being opened to start a code review and then complete the support of this command line and service. There are some necessary manual steps required as part of the larger migration in that the repo is migrated and secret scanning has been reactivated (and the backfill scan completed) before this command line makes sense to execute.

[github-actions[bot]]

Unit Test Results

620 tests   620 :heavy_check_mark:  21s :stopwatch:     1 suites      0 :zzz:     1 files        0 :x:

Results for commit 0858a807.

:recycle: This comment has been updated with latest results.

[peter-murray]

I rebased the changes on the the current main and addressed the various comments from the review in this commit.

I have left the DTO objects in place for now (https://github.com/github/gh-gei/pull/592#discussion_r953038706) as these are useful for building the processing logic and this is the first of the two/maybe three command lines that may need these objects. The are not completely flat objects either and have optional complex nested parts that are relevant if we need to go deeper into performing matches above the simpler logic that is in place now.

[peter-murray]

@ArinGhazarian when I run dotnet format I am only getting a single response about CA1720, is there something I am missing from this, that was invoking it from the src directory?

$ dotnet format
Unable to fix CA1720. No associated code fix found.

[ArinGhazarian]

@ArinGhazarian when I run dotnet format I am only getting a single response about CA1720, is there something I am missing from this, that was invoking it from the src directory?

$ dotnet format
Unable to fix CA1720. No associated code fix found.

It is a compiler warning and it's because there is a property named as Guid (which is guess is a DTO): Warning: /home/runner/work/gh-gei/gh-gei/src/Octoshift/Models/GithubCodeScanningAnalysis.cs(29,19): warning CA1720: Identifier 'Guid' contains type name. I guess you can suppress the warning for that one property:

#pragma warning disable CA1720
// The code that's violating the rule is on this line.
#pragma warning restore CA1720

P.S: I still believe that creating those strongly typed DTOs is a bit of an overkill since most of those properties won't be used anyways.

[peter-murray]

The code scanning cli tool that David is working on will be using the model objects to link up and detect the right scanning results to export and import, so I have suppressed the checks on that model object as indicated. Maybe it can be refactored out in his changes, but for now there is code he is working on that will likely use it.

[peter-murray]

As for the CI build failures, has there been some changes in the main branch that I rebased on in this area as the failures seem unrelated to the code changes I have directly made here?

[ArinGhazarian]

As for the CI build failures, has there been some changes in the main branch that I rebased on in this area as the failures seem unrelated to the code changes I have directly made here?

INT tests are a bit flaky today, I would re-run the jobs.

[peter-murray]

So the tests are now passing, but when I run the command locally now, from Rider, the --github-source-org even when specified on the command line, is not being passed through to the service. It is marked as required in the options and the options parsing is not failing, but the value is empty and I am capturing it as empty here; https://github.com/github/gh-gei/pull/592/files#diff-ae9bc09e955cefc7c22a65ae1d74f07c2281734b8efc099e9ee9fe53d53d412cR118 and failing

Looking at this and comparing to other commands I cannot see what is different, and the other parameters are coming through fine, it is just the source and target organisation names are not coming through from the command line?

If I rename the parameter from --github-source-org (which is what is used elsewhere) to --source-org it works, so have we got something special in place for --github prefixed options now?

[ArinGhazarian]

So the tests are now passing, but when I run the command locally now, from Rider, the --github-source-org even when specified on the command line, is not being passed through to the service. It is marked as required in the options and the options parsing is not failing, but the value is empty and I am capturing it as empty here; https://github.com/github/gh-gei/pull/592/files#diff-ae9bc09e955cefc7c22a65ae1d74f07c2281734b8efc099e9ee9fe53d53d412cR118 and failing

Looking at this and comparing to other commands I cannot see what is different, and the other parameters are coming through fine, it is just the source and target organisation names are not coming through from the command line?

If I rename the parameter from --github-source-org (which is what is used elsewhere) to --source-org it works, so have we got something special in place for --github prefixed options now?

The way model binding works in the version of System.CommandLine we're using is by convention which matches the Option name with the Arg name. So you need to either rename the SourceOrg in the MigrateSecretScanningAlertsCommandArgs model to GithubSourceOrg or as you already mentioned do it the other way around. Now given the fact that in all of our other commands we used --source-org we should do the same here and just rename the Option name to be --srouce-org.

[peter-murray]

I am happy to go with source-org, but that was not the case in the examples that I was pointed to to base off of at the start. Thank you for the clarification though and source-org does better align in this case.

[davelosert]

@ArinGhazarian : Kindly asking if there is any timeline on when this might be merged?

I am already done with the code for the next step (code scanning alert migration) as defined in #535, but it is based on the work that @peter-murray is doing in this PR, so I am blocked with putting up the PR until this is merged.

[dylan-smith]

@ArinGhazarian : Kindly asking if there is any timeline on when this might be merged?

I am already done with the code for the next step (code scanning alert migration) as defined in #535, but it is based on the work that @peter-murray is doing in this PR, so I am blocked with putting up the PR until this is merged.

Hi @davelosert , Arin is unavailable for the next week or two (on-call rotation), but I'll take a look at this on Thursday. Sorry it's taking so long.

[peter-murray]

@dylan-smith friendly bump on the progress of getting this merged?

[peter-murray]

@dylan-smith sent an invite for you on the source repo so you can push changes

[github-actions[bot]]

Integration Test Results

5 tests  ±0   5 :heavy_check_mark: ±0   14m 48s :stopwatch: - 1m 1s 1 suites ±0   0 :zzz: ±0  1 files   ±0   0 :x: ±0 

Results for commit 0858a807. ± Comparison against base commit 9e549e5e.

:recycle: This comment has been updated with latest results.

[github-actions[bot]]

Package	Line Rate	Branch Rate	Complexity	Health
bbs2gh	74%	67%	426	➖
ado2gh	84%	78%	684	✔
gei	84%	79%	572	✔
Octoshift	86%	70%	1004	✔
Summary	84% (5785 / 6920)	74% (1379 / 1874)	2686	✔

[timrogers]

@peter-murray Could you provide a bit more context here on why we skip open alerts? I've seen this come up in a customer support ticket.

[dylan-smith]

@peter-murray Could you provide a bit more context here on why we skip open alerts? I've seen this come up in a customer support ticket.

It’s because if the alert is open there is nothing to migrate. When a secret scan is done on the target you’ll end up with the same open alert without needing to migrate anything.

we’re only migrating closed state and reason.

[timrogers]

@peter-murray Could you provide a bit more context here on why we skip open alerts? I've seen this come up in a customer support ticket.

It’s because if the alert is open there is nothing to migrate. When a secret scan is done on the target you’ll end up with the same open alert without needing to migrate anything.

we’re only migrating closed state and reason.

Thanks! That makes sense. That's the reason I guessed, but I just wanted to validate that.

We should add some documentation for this command to explain a bit more of how it works. We have the built-in CLI documentation, but it doesn't go into much detail, and I've seen that this can confuse customers.