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

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

Merged
merged 30 commits into from
Jun 26, 2023
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
9ce116f
Adding error handling, cleaning up code per last PR, added additional…
hfishback01 May 26, 2023
2763058
Fix verify-no-changes errors
hfishback01 May 26, 2023
7f3bd6d
update api call
hfishback01 May 26, 2023
07cf29b
Merge branch 'main' into reclaimation-error-handling
hfishback01 May 26, 2023
3cfe13e
address new verify-no-changes error
hfishback01 May 26, 2023
81368d6
fix tests
hfishback01 May 26, 2023
93f2918
remove #regions and XML comment
hfishback01 May 26, 2023
4a7aa73
Merge branch 'main' into reclaimation-error-handling
brianaj May 31, 2023
ccec1ea
Merge branch 'main' into reclaimation-error-handling
synthead Jun 2, 2023
f015a2f
Merge branch 'main' into reclaimation-error-handling
hfishback01 Jun 19, 2023
36c83ba
Add --no-prompt option
hfishback01 Jun 20, 2023
0c220ef
address PR feedback less error vs warning
hfishback01 Jun 22, 2023
c939758
update verbiage per Tim
hfishback01 Jun 22, 2023
cfca6c6
change logerror to logwarning when appropriate
hfishback01 Jun 22, 2023
ec80960
update tests
hfishback01 Jun 22, 2023
40dd323
Merge branch 'main' into reclaimation-error-handling
hfishback01 Jun 22, 2023
018e348
update with FF warning
hfishback01 Jun 23, 2023
48d433d
update verbiage so that default is N on prompt
hfishback01 Jun 23, 2023
9dfd63e
update postasync to graphql call and split parsing/validation up
hfishback01 Jun 24, 2023
7e92a61
update error handling per Dylan
hfishback01 Jun 26, 2023
73538a2
Merge branch 'main' into reclaimation-error-handling
hfishback01 Jun 26, 2023
e4238bc
"add feature to allow reclaimation of single mannequin without invita…
hfishback01 Jun 26, 2023
8f8c4d4
Remove invalid test
hfishback01 Jun 26, 2023
fc0ae94
update format
hfishback01 Jun 26, 2023
1d91a17
Merge branch 'main' into reclaimation-error-handling
hfishback01 Jun 26, 2023
55af699
Remove unnecessary variable
hfishback01 Jun 26, 2023
5ba7b60
Merge branch 'reclaimation-error-handling' of https://github.com/gith…
hfishback01 Jun 26, 2023
7d55454
Store and restore color in ConfirmationService's _writeToConsoleOut.
synthead Jun 26, 2023
bf2c4eb
Remove extra "?" from ConfirmationServiceTests.
synthead Jun 26, 2023
9a971d1
Skip unused assignment in ReclaimMannequinCommandHandler.
synthead Jun 26, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASENOTES.md
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
- Add additional error handling to `reclaim-mannequin` process
- Removed ability to use `gh gei` to migrate from ADO -> GH. You must use `gh ado2gh` to do this now. This was long since obsolete, but was still available via hidden args - which have now been removed.
- Add `bbs2gh inventory-report` command to write data available for migrations in CSV form
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ public class ReclaimMannequinCommandArgs : CommandArgs
public string MannequinId { get; set; }
public string TargetUser { get; set; }
public bool Force { get; set; }
public bool NoPrompt { get; set; }
[Secret]
public string GithubPat { get; set; }
public bool SkipInvitation { get; set; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ public ReclaimMannequinCommandBase() : base(
Description = "Map the user even if it was previously mapped"
};

public virtual Option<bool> NoPrompt { get; } = new("--no-prompt")
{
Description = "Overrides all prompts and warnings with 'Y' value."
};

public virtual Option<string> GithubPat { get; } = new("--github-pat")
{
Description = "Personal access token of the GitHub target. Overrides GH_PAT environment variable."
Expand Down Expand Up @@ -83,7 +88,7 @@ public override ReclaimMannequinCommandHandler BuildHandler(ReclaimMannequinComm
var reclaimService = new ReclaimService(githubApi, log);
var confirmationService = sp.GetRequiredService<ConfirmationService>();

return new ReclaimMannequinCommandHandler(log, reclaimService, confirmationService);
return new ReclaimMannequinCommandHandler(log, reclaimService, confirmationService, githubApi);
}

protected void AddOptions()
Expand All @@ -94,6 +99,7 @@ protected void AddOptions()
AddOption(MannequinId);
AddOption(TargetUser);
AddOption(Force);
AddOption(NoPrompt);
AddOption(GithubPat);
AddOption(SkipInvitation);
AddOption(Verbose);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,17 @@ public class ReclaimMannequinCommandHandler : ICommandHandler<ReclaimMannequinCo
private readonly OctoLogger _log;
private readonly ReclaimService _reclaimService;
private readonly ConfirmationService _confirmationService;
private readonly GithubApi _githubApi;

internal Func<string, bool> FileExists = path => File.Exists(path);
internal Func<string, string[]> GetFileContent = path => File.ReadLines(path).ToArray();

public ReclaimMannequinCommandHandler(OctoLogger log, ReclaimService reclaimService, ConfirmationService confirmationService)
public ReclaimMannequinCommandHandler(OctoLogger log, ReclaimService reclaimService, ConfirmationService confirmationService, GithubApi githubApi)
{
_log = log;
_reclaimService = reclaimService;
_confirmationService = confirmationService;
_githubApi = githubApi;
}

public async Task Handle(ReclaimMannequinCommandArgs args)
Expand All @@ -29,6 +31,24 @@ public async Task Handle(ReclaimMannequinCommandArgs args)
throw new ArgumentNullException(nameof(args));
}

if (args.SkipInvitation)
{
// Check if user is admin to EMU org
var login = await _githubApi.GetLoginName();

var membership = await _githubApi.GetOrgMembershipForUser(args.GithubOrg, login);

if (membership != "admin")
{
throw new OctoshiftCliException($"User {login} is not an org admin and is not eligible to reclaim mannequins with the --skip-invitation feature.");
}

if (!args.NoPrompt)
{
_ = _confirmationService.AskForConfirmation("Reclaiming mannequins with the --skip-invitation option is immediate and irreversible. Are you sure you wish to continue? [y/N]");
}
}

if (!string.IsNullOrEmpty(args.Csv))
{
_log.LogInformation("Reclaiming Mannequins with CSV...");
Expand All @@ -38,24 +58,14 @@ public async Task Handle(ReclaimMannequinCommandArgs args)
throw new OctoshiftCliException($"File {args.Csv} does not exist.");
}

//TODO: Get verbiage approved
if (args.SkipInvitation)
{
_ = _confirmationService.AskForConfirmation("Reclaiming mannequins with the --skip-invitation option is immediate and irreversible. Are you sure you wish to continue? (y/n)");
}

await _reclaimService.ReclaimMannequins(GetFileContent(args.Csv), args.GithubOrg, args.Force, args.SkipInvitation);
}
else
{
if (args.SkipInvitation)
{
throw new OctoshiftCliException($"--csv must be specified to skip reclaimation email");
}

_log.LogInformation("Reclaiming Mannequin...");

await _reclaimService.ReclaimMannequin(args.MannequinUser, args.MannequinId, args.TargetUser, args.GithubOrg, args.Force);
await _reclaimService.ReclaimMannequin(args.MannequinUser, args.MannequinId, args.TargetUser, args.GithubOrg, args.Force, args.SkipInvitation);
}
}
}
36 changes: 18 additions & 18 deletions src/Octoshift/Services/ConfirmationService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,61 +3,61 @@ namespace OctoshiftCLI.Services
{
public class ConfirmationService
{
# region Variables

private readonly Action<string> _writeToConsoleOut;
private readonly Action<string, ConsoleColor> _writeToConsoleOut;
private readonly Func<ConsoleKey> _readConsoleKey;
private readonly Action<int> _cancelCommand;

#endregion

#region Constructors
public ConfirmationService()
{
_writeToConsoleOut = msg => Console.WriteLine(msg);
_writeToConsoleOut = (msg, outputColor) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

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) =>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

{
Console.ForegroundColor = outputColor;
Console.WriteLine(msg);
};
_readConsoleKey = ReadKey;
_cancelCommand = code => Environment.Exit(code);
}

// Constructor designed to allow for testing console methods
public ConfirmationService(Action<string> writeToConsoleOut, Func<ConsoleKey> readConsoleKey)
public ConfirmationService(Action<string, ConsoleColor> writeToConsoleOut, Func<ConsoleKey> readConsoleKey, Action<int> cancelCommand)
{
_writeToConsoleOut = writeToConsoleOut;
_readConsoleKey = readConsoleKey;
_cancelCommand = cancelCommand;
}

#endregion

#region Functions
public bool AskForConfirmation(string confirmationPrompt, string cancellationErrorMessage = "")
public virtual bool AskForConfirmation(string confirmationPrompt, string cancellationErrorMessage = "")
{
ConsoleKey response;
do
{
_writeToConsoleOut(confirmationPrompt);
_writeToConsoleOut(confirmationPrompt, ConsoleColor.Yellow);
Console.ForegroundColor = ConsoleColor.White;
response = _readConsoleKey();
if (response != ConsoleKey.Enter)
{
_writeToConsoleOut("");
Copy link
Collaborator

Choose a reason for hiding this comment

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

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();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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."

_writeToConsoleOut("", ConsoleColor.White);
}

} while (response is not ConsoleKey.Y and not ConsoleKey.N);

if (response == ConsoleKey.Y)
{
_writeToConsoleOut("Confirmation Recorded. Proceeding...");
_writeToConsoleOut("Confirmation Recorded. Proceeding...", ConsoleColor.White);
return true;
}
else
{
_writeToConsoleOut("Canceling Command...");
throw new OctoshiftCliException($"Command Cancelled. {cancellationErrorMessage}");
_writeToConsoleOut($"Command Cancelled. {cancellationErrorMessage}", ConsoleColor.White);
_cancelCommand(0);
}
return false;
}

private ConsoleKey ReadKey()
{
return Console.ReadKey(false).Key;
}
#endregion
}
}

60 changes: 55 additions & 5 deletions src/Octoshift/Services/GithubApi.cs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,48 @@ public virtual async Task RemoveTeamMember(string org, string teamSlug, string m
await _retryPolicy.Retry(() => _client.DeleteAsync(url));
}

public virtual async Task<string> GetLoginName()
{
var url = $"{_apiUrl}/graphql";

var payload = new
{
query = "query{viewer{login}}"
};

try
{
return await _retryPolicy.Retry(async () =>
{
var data = await _client.PostGraphQLAsync(url, payload);

return (string)data["data"]["viewer"]["login"];
});
}
catch (Exception ex)
{
throw new OctoshiftCliException($"Failed to lookup the login for current user", ex);
}
Comment on lines +125 to +128

Check notice

Code scanning / CodeQL

Generic catch clause

Generic catch clause.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

}

public virtual async Task<string> GetOrgMembershipForUser(string org, string member)
{
var url = $"{_apiUrl}/orgs/{org}/memberships/{member}";

try
{
var response = await _client.GetAsync(url);

var data = JObject.Parse(response);

return (string)data["role"];
}
catch (HttpRequestException ex) when (ex.StatusCode == HttpStatusCode.NotFound) // Not a member
{
return null;
}
}

public virtual async Task<bool> DoesRepoExist(string org, string repo)
{
var url = $"{_apiUrl}/repos/{org.EscapeDataString()}/{repo.EscapeDataString()}";
Expand Down Expand Up @@ -729,7 +771,7 @@ ... on User {
return data.ToObject<CreateAttributionInvitationResult>();
}

public virtual async Task<ReattributeMannequinToUserResult> ReclaimMannequinsSkipInvitation(string orgId, string mannequinId, string targetUserId)
public virtual async Task<ReattributeMannequinToUserResult> ReclaimMannequinSkipInvitation(string orgId, string mannequinId, string targetUserId)
{
var url = $"{_apiUrl}/graphql";
var mutation = "mutation($orgId: ID!,$sourceId: ID!,$targetId: ID!)";
Expand Down Expand Up @@ -758,10 +800,18 @@ ... on User {
variables = new { orgId, sourceId = mannequinId, targetId = targetUserId }
};

var response = await _client.PostAsync(url, payload);
var data = JObject.Parse(response);

return data.ToObject<ReattributeMannequinToUserResult>();
try
{
return await _retryPolicy.Retry(async () =>
{
var data = await _client.PostGraphQLAsync(url, payload);
return data.ToObject<ReattributeMannequinToUserResult>();
});
}
catch (OctoshiftCliException ex) when (ex.Message.Contains("Field 'reattributeMannequinToUser' doesn't exist on type 'Mutation'"))
{
throw new OctoshiftCliException($"Reclaiming mannequins with the--skip - invitation flag is not enabled for your GitHub organization.For more details, contact GitHub Support.", ex);
}
}

public virtual async Task<IEnumerable<GithubSecretScanningAlert>> GetSecretScanningAlertsForRepository(string org, string repo)
Expand Down
Loading