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

If reauth is required, show the full command-line path of the binary #47

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

rogerhu
Copy link
Contributor

@rogerhu rogerhu commented Sep 3, 2024

The error message that gets printed out tells me to login again. However, it doesn't show the full path of the binary that was used to run the credential helper.

Copy link
Contributor

@minor-fixes minor-fixes left a comment

Choose a reason for hiding this comment

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

Instructions say to put engflow_auth on $PATH, so IMO it's fair and more user-friendly to carry this assumption.

It's a bit less convenient for us, if we're working with dev builds - we have to know what we're doing there, instead of following the error message blindly. But I'd still prefer the simpler perceived UX for users.


On second thought - maybe we can see if the current application is on $PATH, and display the full path if it isn't?

@jayconrod
Copy link
Contributor

I think what @rogerhu is doing here is the best option: print os.Args[0].

If the user installed in PATH and invoked as engflow_auth, great, we report that.

If the user invoked with an absolute path (whether or not engflow_auth is in PATH), then we report that full path.

@rogerhu
Copy link
Contributor Author

rogerhu commented Sep 3, 2024

On second thought - maybe we can see if the current application is on $PATH, and display the full path if it isn't?

Addressing your concerns. Pushed some changes. =)

@rogerhu
Copy link
Contributor Author

rogerhu commented Sep 3, 2024

I think what @rogerhu is doing here is the best option: print os.Args[0].

If the user installed in PATH and invoked as engflow_auth, great, we report that.

If the user invoked with an absolute path (whether or not engflow_auth is in PATH), then we report that full path.

Ok, I'll keep it as is. =)

@rogerhu rogerhu enabled auto-merge (squash) September 3, 2024 17:08
@rogerhu rogerhu merged commit bab0014 into main Sep 3, 2024
10 checks passed
@rogerhu rogerhu deleted the rogerh/reauth branch September 3, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants