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

Allow reading github token from path #239

Closed
wants to merge 1 commit into from

Conversation

h7x4
Copy link

@h7x4 h7x4 commented Jul 23, 2023

Hello!

This is an attempt at fixing #238. I had a look into urfave/cli, but it seems like the FilePath option they provide, only allows you to set default values from files, not override functionality.

I'm a little bit unsure about the error logging. The logger seems to depend on the arguments, and the arguments (would) depend on the logger. I ended up just using standard fmt commands because I don't think it's worth the complexity, trying to partially parse arguments to set up a logger and then parse the rest. I could move the action function behind the part where the logger is created, but I think it makes sense to keep it as part of the command validation.

I have not written any go code before, so if something looks weird, don't assume I had my reasons 😄

Closes #238

@h7x4
Copy link
Author

h7x4 commented Jul 24, 2023

On second thoughts, the way this is set up now can cause some confusing behaviour. If you were to write a path to a non-existing file or location, it would just assume the string to be the token itself, and try to use it to authenticate. Maybe I should rethink this a bit

@tboerger
Copy link
Member

I got to think about it a little bit, I am also thinking about just replacing urfave/cli.

@h7x4
Copy link
Author

h7x4 commented Jul 25, 2023

Do you plan on letting this PR through first, or should I wait until you've replaced it?

@tboerger
Copy link
Member

I need some time to think about the alternative ways, I disliked already the single flag used by the GitHub app, if this gets merged the users could start using it the same way which would result in breaking changes if I change it again to a better solution. Give me some time to come back to this PR.

@h7x4 h7x4 marked this pull request as draft July 28, 2023 19:38
@tboerger
Copy link
Member

Thanks for you effort, I have replaced it by #245 now.

@tboerger tboerger closed this Oct 20, 2023
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.

Allow reading GitHub token from path
2 participants