Skip to content

[gen-csv] Add interactive prompts to generate csv#2891

Merged
varshaprasad96 merged 1 commit intooperator-framework:masterfrom
varshaprasad96:gen-bundle/subcommand
May 6, 2020
Merged

[gen-csv] Add interactive prompts to generate csv#2891
varshaprasad96 merged 1 commit intooperator-framework:masterfrom
varshaprasad96:gen-bundle/subcommand

Conversation

@varshaprasad96
Copy link
Copy Markdown
Member

This PR introduces the feature to provide interactive prompts
to the user to obtain csv metadata.

Description of the change:
Add interactive prompts while generating csv.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 23, 2020
@varshaprasad96
Copy link
Copy Markdown
Member Author

varshaprasad96 commented Apr 23, 2020

Would be helpful to have some reviews. Yet to write unit tests.

Comment thread cmd/operator-sdk/generate/csv.go Outdated
Comment thread cmd/operator-sdk/generate/csv_prompt_util.go Outdated
Comment thread cmd/operator-sdk/generate/csv_prompt_util.go Outdated
Comment thread cmd/operator-sdk/generate/csv_prompt_util.go Outdated
Comment thread cmd/operator-sdk/generate/csv.go Outdated
Comment thread cmd/operator-sdk/generate/csv.go Outdated
Comment thread cmd/operator-sdk/generate/csv.go Outdated
@varshaprasad96 varshaprasad96 force-pushed the gen-bundle/subcommand branch from bf6fc8c to 0ada52e Compare April 24, 2020 17:51
@varshaprasad96 varshaprasad96 changed the title [wip] Add interactive prompts to generate csv [gen-csv] Add interactive prompts to generate csv Apr 24, 2020
@varshaprasad96 varshaprasad96 marked this pull request as ready for review April 24, 2020 17:55
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 24, 2020
@varshaprasad96 varshaprasad96 force-pushed the gen-bundle/subcommand branch from 0ada52e to d9b07f3 Compare April 26, 2020 06:38
@varshaprasad96
Copy link
Copy Markdown
Member Author

Added tests.

Copy link
Copy Markdown
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

I'd like to see a generalized interactive prompt library somewhere like internal/util/projutil so we can reuse it easily and not have to encode Generator-specific logic in cmd/operator-sdk.

Comment thread cmd/operator-sdk/generate/csv.go Outdated
@varshaprasad96 varshaprasad96 force-pushed the gen-bundle/subcommand branch from e28a545 to 1cffae5 Compare April 28, 2020 22:13
@varshaprasad96
Copy link
Copy Markdown
Member Author

Refactored the code to address the comments.
@estroz Can you please take a look at it.

@varshaprasad96 varshaprasad96 force-pushed the gen-bundle/subcommand branch from 1cffae5 to 2493ef8 Compare April 28, 2020 22:23
Comment thread internal/generate/olm-catalog/csv.go Outdated
@varshaprasad96 varshaprasad96 force-pushed the gen-bundle/subcommand branch from 2493ef8 to 5207bf3 Compare April 30, 2020 21:17
Comment thread cmd/operator-sdk/generate/csv.go Outdated
@jmccormick2001
Copy link
Copy Markdown
Contributor

I tested this and it seems to work, I'd say it needs a website docs update and a changelog fragment file?

@varshaprasad96 varshaprasad96 force-pushed the gen-bundle/subcommand branch from 5207bf3 to 185be51 Compare May 1, 2020 22:42
@varshaprasad96
Copy link
Copy Markdown
Member Author

varshaprasad96 commented May 1, 2020

@jmccormick2001 I have added the changelog fragment file to this PR. Will create a follow-up PR to update the docs after this is merged.

Comment thread internal/generate/olm-catalog/csv_cmd_prompt.go Outdated
Comment thread internal/generate/olm-catalog/csv.go Outdated
@varshaprasad96 varshaprasad96 force-pushed the gen-bundle/subcommand branch 2 times, most recently from b499a73 to 0b374d3 Compare May 5, 2020 18:25
@varshaprasad96
Copy link
Copy Markdown
Member Author

@estroz @camilamacedo86 Addressed review comments.

Comment thread cmd/operator-sdk/generate/csv.go
Comment thread cmd/operator-sdk/generate/csv.go Outdated
Comment thread internal/generate/olm-catalog/csv_cmd_prompt.go Outdated
Comment thread internal/util/projutil/interactive_promt_util_test.go Outdated
Comment thread internal/util/projutil/interactive_promt_util_test.go
@varshaprasad96 varshaprasad96 force-pushed the gen-bundle/subcommand branch from 0b374d3 to a37b863 Compare May 5, 2020 20:19
Copy link
Copy Markdown
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

A few suggestions.

Comment thread internal/generate/olm-catalog/csv_cmd_prompt.go Outdated
Comment thread internal/generate/olm-catalog/csv_cmd_prompt.go Outdated
Comment thread internal/generate/olm-catalog/csv_cmd_prompt.go Outdated
Comment thread cmd/operator-sdk/generate/csv.go Outdated
@varshaprasad96 varshaprasad96 force-pushed the gen-bundle/subcommand branch 2 times, most recently from cd0df7c to 8629bf4 Compare May 5, 2020 21:25
Copy link
Copy Markdown
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2020
Copy link
Copy Markdown
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

Looks great! Mostly non-blocking nits.

I think spacing the prompts out would be my only blocker if there's agreement on that suggestion.

Another non-blocking follow-up to this could be to add support for default values. That way if someone has an existing CSV and they ask for --interactive, we can do something like this:

Display name for operator [Example Database Operator]:
>

And empty input would result in the default being used.

Lastly, was there any consideration for using an existing interactive prompt library? I'm not suggesting that we make that change here, but curious if we may want to think about that in the future if we need more features?

Comment thread internal/generate/olm-catalog/csv.go Outdated
Comment thread internal/generate/olm-catalog/csv_cmd_prompt.go Outdated
Comment thread internal/generate/olm-catalog/csv_cmd_prompt.go Outdated
Comment thread internal/generate/olm-catalog/csv_cmd_prompt.go Outdated
Comment thread internal/generate/olm-catalog/csv_cmd_prompt.go Outdated
Comment thread internal/util/projutil/interactive_promt_util.go
Comment thread internal/util/projutil/interactive_promt_util.go Outdated
Comment thread internal/util/projutil/interactive_promt_util.go Outdated
Comment thread internal/generate/olm-catalog/csv_cmd_prompt.go Outdated
@varshaprasad96 varshaprasad96 force-pushed the gen-bundle/subcommand branch from 8629bf4 to 688ceb8 Compare May 6, 2020 05:20
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 6, 2020
@openshift-ci-robot
Copy link
Copy Markdown

New changes are detected. LGTM label has been removed.

@varshaprasad96
Copy link
Copy Markdown
Member Author

varshaprasad96 commented May 6, 2020

@joelanford I completely agree with the formatting, and have modified the PR so that the prompts have spaces in between. There are a few prompt libraries like go-prompt and prompt-ui. These are shell libraries which have more advanced features like auto-complete/suggestions or selections . And for creating simple prompts as we have done, they follow a similar approach as here. To make it more flexible (like getting strings, and performing any further operations on it), I wrote these functions to do the same. But we could definitely modify the available libraries in future to have more powerful features and modifications in the prompts. I'll create a follow up PR for providing the default values to the user when we have base CSV.

@varshaprasad96 varshaprasad96 force-pushed the gen-bundle/subcommand branch 2 times, most recently from 4be7b11 to 7703efa Compare May 6, 2020 05:47
Copy link
Copy Markdown
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

One more nit, but otherwise LGTM!

GREAT job on this! Totally digging the design of the CLI prompt library and keeping it separate from the CSV generation code that uses it.

Comment thread internal/generate/olm-catalog/csv_cmd_prompt.go Outdated
This PR introduces the feature to provide interactive prompts
to the user to obtain csv metadata.
@varshaprasad96 varshaprasad96 force-pushed the gen-bundle/subcommand branch from 7703efa to 5366a39 Compare May 6, 2020 16:08
@varshaprasad96 varshaprasad96 merged commit 4916e6a into operator-framework:master May 6, 2020
@varshaprasad96 varshaprasad96 deleted the gen-bundle/subcommand branch May 22, 2020 22:24
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.

7 participants