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

A few Proposal(s) : CLI args improvement, GitHub Enterprise support #11

Open
tprasadtp opened this issue Nov 11, 2019 · 2 comments
Open
Labels
discussion Issues for discussing ideas for features

Comments

@tprasadtp
Copy link
Contributor

tprasadtp commented Nov 11, 2019

Hey @hackebrot
I would like to propose the following changes

  1. Remove -u | --username option and use token via HTTP headers.
    It is not necessary and github will soon stop supporting password based authentication to its APIs. username is not necessary if HTTP headers are used, see this.

  2. This might be better suited under bugs. Because how click args are defined, --help is useless
    for fetch and sync without specifying -t | --token as it gives an error.

      Usage: labels [OPTIONS] COMMAND [ARGS]...
      Try "labels --help" for help.
    
      Error: Missing option "-t" / "--token".
    
  3. Support GitHub enterprise
    I think adding an optional argument which defaults to api.github.com should be sufficient. (for SSL system certs should be used, as its the most sane option)

  4. for option above, and for the token I would like to propose that we use GITHUB_URL and GITHUB_TOKEN as env variables as they are better in my opinion than LABELS_TOKEN

  5. Because above changes are breaking changes we need to bump minor version

  6. Docker
    docker
    I am not proposing that you provide docker images. because I already do it anyway and will be happy to maintain the images on GitHub and DockerHub.

  7. May be I am spoiled by Go. Use Pyinstaller for packaging it into a binary? its always nice to have a self contained static binary.(I know about libc requirements) Though, I am not too keen on this one.

  8. This one may be my peeve, isort and black for formatting?

I have already completed work on most of them, (needs a bit more testing for GitHub Enterprise) and will be happy to submit PR(s) if you are willing to accept any of these.

@tprasadtp tprasadtp changed the title A few Proposal(s) : CLI args improvement, GitHub Enterprise supports A few Proposal(s) : CLI args improvement, GitHub Enterprise support Nov 11, 2019
@hackebrot
Copy link
Owner

Hi @tprasadtp! 👋

Thank you for your suggestions. Here are my thoughts on the individual items:

  1. We currently use HTTP basic authentication and accept a username and a token. It is my understanding that this method is still supported and not subject of the announced deprecations

  2. I believe that is how sub-commands are designed in click. Can you please describe what a help message you would expect?

  3. I'm happy to review pull-requests for adding a new CLI option for the API URL. 👍

  4. As a user, I would find it more helpful if labels tells me that it's missing a required parameter, rather than it silently loading generic environment variables. Chances are that people already have a GITHUB_TOKEN that is not set to the token with the correct scope for labels.

  5. Let's not introduce breaking changes unless it's strictly necessary. ⚠️

  6. Sure, we can build a Docker image with GitHub actions and publish to GitHub Packages. 🤖

  7. What would be the benefit of a self contained static binary over Installing the package from PyPI or pulling the Docker image from GitHub?

  8. The labels source code is formatted with black. We could add pre-commit or automation for isort and black though. 🙂

@hackebrot hackebrot added the discussion Issues for discussing ideas for features label Nov 14, 2019
@tprasadtp
Copy link
Contributor Author

tprasadtp commented Nov 14, 2019

Thank you,

We currently use HTTP basic authentication and accept a username and a token. It is my understanding that this method is still supported and not subject of the announced deprecations

Yes, because we use tokens, username is not necessary if we authenticate with headers. I thought it would be better because that's one less argument to specify https://developer.github.com/v3/#oauth2-token-sent-in-a-header

I believe that is how sub-commands are designed in click. Can you please describe what a help message you would expect?

I understand that. I would expect the following to be the help message. But to see that I have to specify -u and -t

labels -t 1bc -u none fetch --help
Usage: labels fetch [OPTIONS]

  Fetch labels for a GitHub repository.

  This will write the labels information to disk to the specified filename.

Options:
  -o, --owner TEXT     GitHub owner name  [required]
  -r, --repo TEXT      GitHub repository name  [required]
  -f, --filename PATH  Filename for labels  [required]
  --help               Show this message and exit.

I am not exactly sure how to overcome that other than making them arguments to subcommands.
as its click's limitation.

I'm happy to review pull-requests for adding a new CLI option for the API URL

I need to test it with custom CA and then I will submit a PR.

Sure, we can build a Docker image with GitHub actions and publish to GitHub Packages.

I just found out that GitHub packages does not support un-authenticated docker pulls yet.:(

What would be the benefit of a self contained static binary over Installing the package from PyPI or pulling the Docker image from GitHub?

If docker images are provided then I don't think its necessary.

The labels source code is formatted with black. We could add pre-commit or automation for isort and black though.

That's a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Issues for discussing ideas for features
Projects
None yet
Development

No branches or pull requests

2 participants