Add error handling, general refinement to `--skip-invitation` flag #1006

hfishback01 · 2023-05-26T15:56:35Z

Closes #6913
Closes #6914
Closes #1011

This PR addresses feedback from previous PR, including:

Moves csv specification check to validate method in the args file
Removes unnecessary comments
Updated specific unit tests

It also adds error handling and unit testing for different scenarios as outlined by screenshots below. I've also created an issue to update error message handling here when GH is updated:

--check for duplicate claimant in same issue errror

Updates console color for confirmation request also exits more gracefully on "n":

Errors handled:

Handles duplicates (both to same and different claimants):

User not admin to EMU org:

Other errors that were already handled but mentioned in the issue:

Mannequins already claimed:

[2023-05-23 17:29:12] [ERROR] brianaj is already claimed. Skipping (use force if you want to reclaim)

Mannequins not part of target org

[2023-05-23 17:58:36] [ERROR] Mannequin gargola not found. Skipping.

Claimant not part of target org:

[2023-05-23 17:40:08] [ERROR] Failed to reclaim dpmex4527 (M_kgDOCAN3aw) to hfishback01 (MDQ6VXNlcjM4ODQ1Mjcy): Target must be a member of the hfishback01-cli-test3 organization

--no-prompt Used:

Feature Flag not enabled:

Did you write/update appropriate tests
Release notes updated (if appropriate)
Appropriate logging output
Issue linked
Docs updated (or issue created)
New package licenses are added to ThirdPartyNotices.txt (if applicable)

… unit testing

github-actions · 2023-05-26T16:43:10Z

Unit Test Results

785 tests 785 ✔️ 23s ⏱️
    1 suites     0 💤
    1 files     0 ❌

Results for commit 9a971d1.

♻️ This comment has been updated with latest results.

src/Octoshift/Services/ReclaimService.cs

src/OctoshiftCLI.Tests/Octoshift/Services/ConfirmationServiceTests.cs

brianaj · 2023-05-26T23:39:10Z

Thanks @hfishback01 for getting this out, can you answer if you handled if org is not emu, not sure if this was addressed or not didn't catch it in the description/screenshots. If not can you add a line item to finish as part of this PR as that's a main error scenario we'll want to handle and make sure we don't keep iterating through csv.

src/Octoshift/Services/ReclaimService.cs

synthead

Looking great! I have a few PR review items to take a look at!

Also, since we're now having interactive prompts, we should probably add a --no-prompt option, too. We promote this tool to be used in scripts (it even creates said scripts!) so we should be ready to handle non-interactive commands.

We could start by going the easy route and abort with an error stating that --no-prompt was used, and there are unresolved mannequins that the user needs to address. This should be enough to have the user read the logs and know exactly what to do 👍

If we aren't implementing the mannequin reclaiming command in any scripts, and we're sure that the scripts generated from the CLI tool won't hang, I'm 100% on making this a tech debt item 👍

src/Octoshift/Services/GithubApi.cs

src/Octoshift/Services/ReclaimService.cs

RELEASENOTES.md

src/Octoshift/Services/ConfirmationService.cs

src/OctoshiftCLI.Tests/Octoshift/Services/ConfirmationServiceTests.cs

src/Octoshift/Commands/ReclaimMannequin/ReclaimMannequinCommandHandler.cs

src/Octoshift/Services/ReclaimService.cs

RELEASENOTES.md

src/Octoshift/Commands/ReclaimMannequin/ReclaimMannequinCommandArgs.cs

synthead · 2023-06-23T18:56:59Z

src/Octoshift/Services/GithubApi.cs

+        catch (Exception ex)
+        {
+            throw new OctoshiftCliException($"Failed to lookup the login for current user", ex);
+        }


I agree with this CodeQL warning, here. What does a failure look like? Would there be a permission error, for example? Keep in mind that GraphQL will return a 200 on failures related to the response, so we'll want to inspect the GraphQL "errors" key. The errors that come back from GitHub should already be sanitized and helpful, so most of the hard stuff is probably already done for you: see if there's an "errors" key, and if so, append the message to your own exception message! 👌

We're using _retryPolicy here, so it should be robust against network failures, but we should still catch errors related to network issues. If you want to quickly functionally test this, you could redefine url to something you know will not work, like https://my-local-broken-network. The _retryPolicy should kick in and exhaust its retries, then raise an exception related to the network failure.

Additionally, another thing we should be careful of is reporting the correct exceptions to the user. With this block, every exception will turn into a generic OctoshiftCliException error. This means that if a user creates a friction issue with us and mentions the "Failed to lookup the login for current user" error, we will have no information about it by design, and it'd be difficult to help them.

It's probably better to handle most expected cases, like permissions errors being raised via GraphQL and some network-related errors, then go ahead and let the edge cases slip through as unhandled exceptions (in this scope). Then we'll get all the weird stuff displayed in the console and in the logs, even if they aren't following the happiest path of exception handling 👍

@synthead The OctoshiftCliException is set up so that we can pass the exception ("ex") as an inner exception; which the user can view using the --verbose option. :)

This is the pattern used on a few other functions within this class. If we want to get more specific with errors here, should this and other functions be cleaned up as part of a separate PR? 🤷‍♀️

As for "what does a failure" look like here; honestly, I don't think there really will be anything outside of a network error; I set up the catch to be cautious. Since we're just getting the user's login and that is associated with the PAT they are using, I don't think we should run into a permissions issue or anything else particularly funky.

synthead

Looking great! I have a few things for you to 👀 at! Gimme a shout if you'd like to pair!

synthead · 2023-06-23T18:45:23Z

src/Octoshift/Services/ConfirmationService.cs

        public ConfirmationService()
        {
-            _writeToConsoleOut = msg => Console.WriteLine(msg);
+            _writeToConsoleOut = (msg, outputColor) =>


What if we made outputColor default to ConsoleColor.White? That way, we only need to specify a color if we want to use something other than the default (implied ConsoleColor.White).

Something like:

Suggested change

_writeToConsoleOut = (msg, outputColor) =>

_writeToConsoleOut = (msg, outputColor = ConsoleColor.White) =>

This doesn't seem to be a c# feature (yet): https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/lambda-method-group-defaults#relevant-background

Unless we want to use preview but I don't think it's worth using an unsupported feature.

src/Octoshift/Commands/ReclaimMannequin/ReclaimMannequinCommandHandler.cs

synthead · 2023-06-23T18:48:30Z

src/Octoshift/Services/ConfirmationService.cs

                response = _readConsoleKey();
                if (response != ConsoleKey.Enter)
                {
-                    _writeToConsoleOut("");


Is this to write a blank line to the output? If so, perhaps we also want to have msg have an optional parameter that defaults to ""?

_writeToConsoleOut = (msg = "", outputColor = ConsoleColor.White) =>

This way, for empty (white) lines, we can simply do:

_writeToConsoleOut();

Same as the other default value comment... I think it would be a great once this is implemented though!

"This doesn't seem to be a c# feature (yet): https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/lambda-method-group-defaults#relevant-background

Unless we want to use preview but I don't think it's worth using an unsupported feature."

synthead · 2023-06-23T18:56:59Z

src/Octoshift/Services/GithubApi.cs

+        catch (Exception ex)
+        {
+            throw new OctoshiftCliException($"Failed to lookup the login for current user", ex);
+        }


I agree with this CodeQL warning, here. What does a failure look like? Would there be a permission error, for example? Keep in mind that GraphQL will return a 200 on failures related to the response, so we'll want to inspect the GraphQL "errors" key. The errors that come back from GitHub should already be sanitized and helpful, so most of the hard stuff is probably already done for you: see if there's an "errors" key, and if so, append the message to your own exception message! 👌

We're using _retryPolicy here, so it should be robust against network failures, but we should still catch errors related to network issues. If you want to quickly functionally test this, you could redefine url to something you know will not work, like https://my-local-broken-network. The _retryPolicy should kick in and exhaust its retries, then raise an exception related to the network failure.

Additionally, another thing we should be careful of is reporting the correct exceptions to the user. With this block, every exception will turn into a generic OctoshiftCliException error. This means that if a user creates a friction issue with us and mentions the "Failed to lookup the login for current user" error, we will have no information about it by design, and it'd be difficult to help them.

It's probably better to handle most expected cases, like permissions errors being raised via GraphQL and some network-related errors, then go ahead and let the edge cases slip through as unhandled exceptions (in this scope). Then we'll get all the weird stuff displayed in the console and in the logs, even if they aren't following the happiest path of exception handling 👍

src/Octoshift/Services/ReclaimService.cs

synthead · 2023-06-23T19:18:50Z

src/Octoshift/Services/ReclaimService.cs

+            switch (result.Errors[0].Message)
+            {
+                case string a when a.Contains("is not an Enterprise Managed Users (EMU) organization"):
+                    _log.LogError("Failed to reclaim mannequins. The --skip-invitation flag is only available to EMU organizations.");
+                    return false; // Indicates we should stop parsing through the CSV
+                default:
+                    _log.LogWarning($"Failed to reclaim {mannequinUser} ({mannequin.Id}) to {targetUser} ({targetUserId}): {result.Errors[0].Message}");
+                    return true;
+            }


Is there a way that we can handle this error without parsing human-readable error messages? Does the GraphQL response happen to include a key or something? If so, we should watch for that 👌

If not (and this is difficult to consume as an API user), we should probably file an issue on our end to make this easier to handle 🙂

@synthead At this point, I don't think so.

This is where the error gets raised: https://github.com/github/github/blob/4ad31aba4642a3776068727dd578ca76e61911df/lib/platform/mutations/reattribute_mannequin_to_user.rb#L33

Is there an example of a key somewhere so I can see what you're envisioning?

src/Octoshift/Services/GithubApi.cs

…iton requirement"

src/OctoshiftCLI.Tests/Octoshift/Services/ReclaimServiceTests.cs

…ub/gh-gei into reclaimation-error-handling

brianaj

👍🏾 I think if there are any more non-blocking comments should create a new follow-up issue(s) to address feedback.

co-authored-by: Hannah Fishback <38845272+hfishback01@users.noreply.github.com>

github-actions · 2023-06-26T21:32:53Z

Package	Line Rate	Branch Rate	Complexity	Health
gei	81%	75%	538	✔
ado2gh	84%	82%	617	✔
bbs2gh	79%	76%	665	✔
Octoshift	86%	76%	1208	✔
Summary	83% (6586 / 7900)	77% (1566 / 2036)	3028	✔

Adding error handling, cleaning up code per last PR, added additional…

9ce116f

… unit testing

hfishback01 self-assigned this May 26, 2023

Fix verify-no-changes errors

2763058

hfishback01 and others added 4 commits May 26, 2023 15:31

update api call

7f3bd6d

Merge branch 'main' into reclaimation-error-handling

07cf29b

address new verify-no-changes error

3cfe13e

fix tests

81368d6

hfishback01 marked this pull request as ready for review May 26, 2023 23:15

dylan-smith reviewed May 26, 2023

View reviewed changes

src/Octoshift/Services/ReclaimService.cs Outdated Show resolved Hide resolved

dylan-smith reviewed May 26, 2023

View reviewed changes

src/Octoshift/Services/ReclaimService.cs Outdated Show resolved Hide resolved

dylan-smith reviewed May 26, 2023

View reviewed changes

src/Octoshift/Services/ReclaimService.cs Outdated Show resolved Hide resolved

dylan-smith reviewed May 26, 2023

View reviewed changes

src/OctoshiftCLI.Tests/Octoshift/Services/ConfirmationServiceTests.cs Outdated Show resolved Hide resolved

hfishback01 commented May 26, 2023

View reviewed changes

src/Octoshift/Services/ReclaimService.cs Show resolved Hide resolved

hfishback01 and others added 2 commits May 26, 2023 16:59

remove #regions and XML comment

93f2918

Merge branch 'main' into reclaimation-error-handling

4a7aa73

brianaj requested a review from dylan-smith May 31, 2023 17:22

brianaj self-assigned this May 31, 2023

synthead self-requested a review May 31, 2023 19:46

synthead reviewed May 31, 2023

View reviewed changes

dylan-smith mentioned this pull request Jun 1, 2023

reclaim-mannequin --skip-invitation option cannot be used in non-interactive environment #1011

Closed

synthead and others added 3 commits June 1, 2023 23:17

Merge branch 'main' into reclaimation-error-handling

ccec1ea

Merge branch 'main' into reclaimation-error-handling

f015a2f

Add --no-prompt option

36c83ba

timrogers reviewed Jun 21, 2023

View reviewed changes

RELEASENOTES.md Outdated Show resolved Hide resolved

src/Octoshift/Commands/ReclaimMannequin/ReclaimMannequinCommandArgs.cs Outdated Show resolved Hide resolved

src/Octoshift/Commands/ReclaimMannequin/ReclaimMannequinCommandArgs.cs Outdated Show resolved Hide resolved

address PR feedback less error vs warning

0c220ef

github-advanced-security bot found potential problems Jun 22, 2023

View reviewed changes

hfishback01 added 2 commits June 22, 2023 14:43

update verbiage per Tim

c939758

change logerror to logwarning when appropriate

cfca6c6

synthead reviewed Jun 23, 2023

View reviewed changes

update with FF warning

018e348

dylan-smith reviewed Jun 23, 2023

View reviewed changes

src/Octoshift/Services/GithubApi.cs Outdated Show resolved Hide resolved

hfishback01 added 2 commits June 23, 2023 15:12

update verbiage so that default is N on prompt

48d433d

update postasync to graphql call and split parsing/validation up

9dfd63e

github-advanced-security bot found potential problems Jun 24, 2023

View reviewed changes

src/Octoshift/Services/GithubApi.cs Fixed Show fixed Hide fixed

hfishback01 requested review from brianaj, dylan-smith and synthead June 24, 2023 01:53

dylan-smith reviewed Jun 24, 2023

View reviewed changes

src/Octoshift/Services/GithubApi.cs Outdated Show resolved Hide resolved

dylan-smith reviewed Jun 24, 2023

View reviewed changes

src/Octoshift/Services/GithubApi.cs Outdated Show resolved Hide resolved

hfishback01 and others added 2 commits June 26, 2023 07:55

update error handling per Dylan

7e92a61

Merge branch 'main' into reclaimation-error-handling

73538a2

hfishback01 requested a review from dylan-smith June 26, 2023 14:01

hfishback01 and others added 4 commits June 26, 2023 10:35

"add feature to allow reclaimation of single mannequin without invita…

e4238bc

…iton requirement"

Remove invalid test

8f8c4d4

update format

fc0ae94

Merge branch 'main' into reclaimation-error-handling

1d91a17

github-advanced-security bot found potential problems Jun 26, 2023

View reviewed changes

src/OctoshiftCLI.Tests/Octoshift/Services/ReclaimServiceTests.cs Fixed Show fixed Hide fixed

hfishback01 added 2 commits June 26, 2023 11:09

Remove unnecessary variable

55af699

Merge branch 'reclaimation-error-handling' of https://github.com/gith…

5ba7b60

…ub/gh-gei into reclaimation-error-handling

brianaj approved these changes Jun 26, 2023

View reviewed changes

synthead and others added 3 commits June 26, 2023 13:46

Store and restore color in ConfirmationService's _writeToConsoleOut.

7d55454

co-authored-by: Hannah Fishback <38845272+hfishback01@users.noreply.github.com>

Remove extra "?" from ConfirmationServiceTests.

bf2c4eb

co-authored-by: Hannah Fishback <38845272+hfishback01@users.noreply.github.com>

Skip unused assignment in ReclaimMannequinCommandHandler.

9a971d1

co-authored-by: Hannah Fishback <38845272+hfishback01@users.noreply.github.com>

synthead requested a review from brianaj June 26, 2023 21:25

brianaj approved these changes Jun 26, 2023

View reviewed changes

hfishback01 merged commit b87d7b4 into main Jun 26, 2023

hfishback01 deleted the reclaimation-error-handling branch June 26, 2023 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add error handling, general refinement to `--skip-invitation` flag #1006

Add error handling, general refinement to `--skip-invitation` flag #1006

hfishback01 commented May 26, 2023 •

edited

Loading

github-actions bot commented May 26, 2023 •

edited

Loading

brianaj commented May 26, 2023

synthead left a comment

synthead Jun 23, 2023

hfishback01 Jun 23, 2023

synthead left a comment

synthead Jun 23, 2023

hfishback01 Jun 23, 2023

synthead Jun 23, 2023

hfishback01 Jun 23, 2023

synthead Jun 23, 2023

synthead Jun 23, 2023

hfishback01 Jun 24, 2023

brianaj left a comment

github-actions bot commented Jun 26, 2023

	_writeToConsoleOut = (msg, outputColor) =>
	_writeToConsoleOut = (msg, outputColor = ConsoleColor.White) =>

Add error handling, general refinement to --skip-invitation flag #1006

Add error handling, general refinement to --skip-invitation flag #1006

Conversation

hfishback01 commented May 26, 2023 • edited Loading

github-actions bot commented May 26, 2023 • edited Loading

Unit Test Results

brianaj commented May 26, 2023

synthead left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

synthead left a 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

brianaj left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 26, 2023

Add error handling, general refinement to `--skip-invitation` flag #1006

Add error handling, general refinement to `--skip-invitation` flag #1006

hfishback01 commented May 26, 2023 •

edited

Loading

github-actions bot commented May 26, 2023 •

edited

Loading