Default to github-app in provider enroll #3032
Conversation
| ProviderCmd.PersistentFlags().StringP("project", "j", "", "ID of the project") | ||
| // TODO: get rid of this | ||
| ProviderCmd.PersistentFlags().StringP("provider", "p", "", "DEPRECATED - use `class` flag of `enroll` instead") |
There was a problem hiding this comment.
Since this was only useful for the provider enroll command, should it be moved there? Otherwise other commands like provider list see the deprecation message
There was a problem hiding this comment.
I left it that way for the sake of backwards compatibility. For example, in the current CLI, it is acceptable to write:
minder provider --provider=github enroll
Which would be broken if we moved that flag down to the leaf node commands as far as I can tell.
I also think that we should consider changing the name of the parameter: in some commands elsewhere in the CLI, provider might mean the name of the provider, here it means the class/type
| @@ -275,8 +283,10 @@ func init() { | |||
| ProviderCmd.AddCommand(enrollCmd) | |||
| // Flags | |||
| enrollCmd.Flags().StringP("token", "t", "", "Personal Access Token (PAT) to use for enrollment") | |||
There was a problem hiding this comment.
The token is also for legacy GitHub only
There was a problem hiding this comment.
for now. Other providers (e.g. DockerHub) will make use of this.
There was a problem hiding this comment.
The problem is that with the current approach, this command will end up with every possible optional flag for every possible provider. In the Minder CLI proposal document, I have suggested that we move to a structure with subcommands per provider, e.g.:
minder provider enroll github-app [flags]
minder provider enroll github [flags]
minder provider enroll oci [flags]
However, since we need to maintain backwards compatibility, I am not making this change at this point.
Ria's feedback does raise the issue of whether we should avoid calling certain parts of the code if we are creating a github-app provider
There was a problem hiding this comment.
Eventually, it would be nice if we could use the AuthorizationFlows for each provider to create a selection box for the user. For example the legacy github provider support the OAuth flow and the user input flow, so when they run minder provider enroll --class github we could show:
Select your authorization flow:
[] provide the token myself
[] use OAuth 2.0 authorization code flow to generate a token
Then we remove --token from the parameters of enroll and new providers just have to define their accepted authorization flows.
Fixes #2898 Some additional changes have been made as well: 1) Make the flag to specify the class of provider specific to the enroll command. The existing top-level provider flag is left intact, but marked as deprecated. 2) The confirmation box is only shown for the legacy Github app workflow as requested in the ticket. 3) The timeout duration is listed in the CLI output when the browser is opened. 4) Small changes to message verbiage here and there.
dmjb
force-pushed
the
default-enroll-github-app
branch
from
45b20cc
to
f70d601
Compare
Fixes #2898
Some additional changes have been made as well:
command. The existing top-level provider flag is left intact, but
marked as deprecated.
as requested in the ticket.
opened.
Summary
Provide a brief overview of the changes and the issue being addressed.
Explain the rationale and any background necessary for understanding the changes.
List dependencies required by this change, if any.
Fixes #(related issue)
Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: