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

feat: add dev preview capability #668

Closed
wants to merge 5 commits into from
Closed

feat: add dev preview capability #668

wants to merge 5 commits into from

Conversation

wtrocki
Copy link
Collaborator

@wtrocki wtrocki commented May 18, 2021

Creating as separate PR to review but this PR targets srs-cli branch that has SDK and initial commands.

Usage:

  1. rhoas help (see no commands)
  2. rhoas --devpreview=true
  3. rhoas help
    Check if commands are there


logger, err := f.Logger()
if err != nil {
logger.Info("Cannot enable dev preview")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will do i18n as last step on the feature branch. Ignore non externalized strings for now

@@ -46,17 +46,25 @@ func NewRootCommand(f *factory.Factory, version string) *cobra.Command {
var help bool
fs.BoolVarP(&help, "help", "h", false, f.Localizer.MustLocalize("root.cmd.flag.help.description"))

var devpreview *string = nil
cmd.Flags().StringVar(devpreview, "devpreview", "", f.Localizer.MustLocalize("root.cmd.flag.devpreview.description"))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I spent most of the time thinking how to enable this properly.
Choose string finally as the best way.
Technically we can validate it for the proper values as well :)
This is as string because:

  • boolean has only 2 possible values - we need 3
  • It will simplify adding tiers later without introducing other flags etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

What will the valid values be?

Copy link
Collaborator Author

@wtrocki wtrocki May 18, 2021

Choose a reason for hiding this comment

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

Open to your feedback.

Currently true, yes etc. but we can rename flag and have alpha beta etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

The thing about string flags is the need an argument so you cannot just do rhoas srs --devpreview - its a bit more awkward to achieve.

Copy link
Collaborator Author

@wtrocki wtrocki May 18, 2021

Choose a reason for hiding this comment

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

Acording to this issue boolean ones are not going to help us

spf13/cobra#613

This command doesn't way as presented. Srs will not be visible. DevPreview flag on it's own modifies config so its not needed when calling dev preview commands.

rhoas srs --devpreview -> rhoas --devpreview=true

--devpreview in this sense provides empty string and means nothing.
If there is way to recognize flag being passed that will be good

cmd.AddCommand(serviceaccount.NewServiceAccountCommand(f))
cmd.AddCommand(cluster.NewClusterCommand(f))
cmd.AddCommand(status.NewStatusCommand(f))
cmd.AddCommand(completion.NewCompletionCommand(f))
cmd.AddCommand(whoami.NewWhoAmICmd(f))
cmd.AddCommand(cliversion.NewVersionCmd(f))

// Dev preview commands
if profile.DevPreviewEnabled(f) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have investigated using custom help message template based on the annotations, however that will require pulling some large parts of the internal cobra functions etc. so decided to simplify it by listing those commands bellow. This will be mostly seen by us so there is no big deal with it.


logger, err := f.Logger()
if err != nil {
logger.Info("Cannot enable dev preview")
Copy link
Contributor

Choose a reason for hiding this comment

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

That will fail because the logger is not created.

cmd.AddCommand(serviceaccount.NewServiceAccountCommand(f))
cmd.AddCommand(cluster.NewClusterCommand(f))
cmd.AddCommand(status.NewStatusCommand(f))
cmd.AddCommand(completion.NewCompletionCommand(f))
cmd.AddCommand(whoami.NewWhoAmICmd(f))
cmd.AddCommand(cliversion.NewVersionCmd(f))

// Dev preview commands
if profile.DevPreviewEnabled(f) {
cmd.AddCommand(registry.NewServiceRegistryCommand(f))
Copy link
Contributor

Choose a reason for hiding this comment

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

❯ ./rhoas --devpreview=yes registry
Error: unknown command "registry" for "rhoas"

How do I get this to work?

@wtrocki
Copy link
Collaborator Author

wtrocki commented May 18, 2021

Update from call. We will refactor this command post POC like in the #346 , however for the moment, I will keep flags this way.

Pushing changes to feature branch.

@wtrocki
Copy link
Collaborator Author

wtrocki commented May 18, 2021

This is still printing help when dev preview is enabled.

@wtrocki wtrocki closed this May 18, 2021
@wtrocki wtrocki deleted the dev-preview branch May 18, 2021 15:06
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.

2 participants