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

(SDK-284) Add guidance for users during new module interview #103

Merged
merged 2 commits into from
Jun 29, 2017

Conversation

rodjek
Copy link
Contributor

@rodjek rodjek commented Jun 23, 2017

Refactors the module metadata interview to use TTY::Prompt. This provides a much nicer user experience when an invalid response is given in that it will blank and reprint the question on the same line rather than reprinting the question on a new line.

Additionally, the questions are now numbered (automatically) and can be specified with some help text to provide guidance to the users. The interview definition has also been separated from the metadata update process in order to make it easier to update the questions in the future.

@rodjek rodjek force-pushed the sdk-284 branch 3 times, most recently from a3c6a8b to 4a44097 Compare June 23, 2017 04:43
@rodjek rodjek changed the title [WIP] (SDK-284) Add guidance for users during new module interview (SDK-284) Add guidance for users during new module interview Jun 23, 2017

interview = PDK::CLI::Util::Interview.new(prompt)

questions.reject! { |q| q[:name] == 'license' } if opts.key?(:license)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice idea!

@scotje
Copy link
Contributor

scotje commented Jun 26, 2017

Love this is general, it does have some slightly sad times on Windows when it repositions the cursor, which is a similar issue to tty-spinner which hopefully will be resolved by ansicon:

PS C:\Users\Administrator> ruby C:\Users\Administrator\src\pdk\bin\pdk new module test_prompt
pdk (INFO): Creating new module: test_prompt

We need to create a metadata.json file for this module, so we're going to ask you 8 quick questions.
If the question is not applicable to this module, just leave the answer blank.

[Q 1/8]
What is your Puppet Forge username?
This will be used when uploading your module to the Forge. You can opt out of this at any time.
←[2K←[1G←[1A←[2K←[1G--> administrator

[Q 2/8]
What version is this module?
Puppet uses Semantic Versioning (semver.org) to version modules.
←[1A←[2K←[1G--> 0.1.0

[Q 3/8]
Who wrote this module?
The person who gets credit for creating the module.
--> (Administrator)

In the meantime perhaps we can add a Windows fallback UI? (Or we can just prioritize getting ansicon into the packaging/system requirements.)

Copy link
Contributor

@scotje scotje left a comment

Choose a reason for hiding this comment

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

Clean Windows output

help: _('This will be used when uploading your module to the Forge. You can opt out of this at any time.'),
required: true,
validate_pattern: %r{\A[a-z0-9]+\Z},
validate_message: _('Forge usernames can only contain letters and numbers'),
Copy link
Contributor

Choose a reason for hiding this comment

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

lowercase letters

Copy link
Contributor

Choose a reason for hiding this comment

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

(Technically the Forge will accept uppercase but I think we want to strongly discourage that, we may even coerce new usernames to all lowercase.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I just stole that pattern from PDK::Module::Metadata's name validation :)

@DavidS
Copy link
Contributor

DavidS commented Jun 27, 2017

I've spent a bit of time today to explore our options. Turns out that full VT100 emulation is supported in the windows console. Starting from Windows 10 TH2 :-/ See https://stackoverflow.com/a/44482740 for details.

As a fallback, @scotje (as well as the SO answer linked above) suggest ansicon, which works fine in my testing. I'll ticket ansicon integration separately. I expect that packaging to be straightforward, so we can merge this in expectation that we'll have ansicon available RSN?

@DavidS DavidS force-pushed the sdk-284 branch 2 times, most recently from 1076bb1 to 58ce32a Compare June 27, 2017 15:19
@DavidS DavidS added the feature label Jun 27, 2017
@scotje
Copy link
Contributor

scotje commented Jun 27, 2017

Yes, the very latest Windows 10 has control sequence support but you do have to opt into it still. :(

@DavidS
Copy link
Contributor

DavidS commented Jun 28, 2017

DavidS@6ec007e has a prototype for supporting Windows' VT100 support. Since this support is so new, we can't depend on it, and need to investigate/implement alternative routes, like ansicon, see SDK-293.

@scotje as this is primarily an aesthetic problem, can we merge this, and revisit a local fallback here vs. using ansicon, in the next few days when we understand better the costs of ansicon?

@scotje
Copy link
Contributor

scotje commented Jun 28, 2017

@DavidS (shrug) We've gone out of our way to provide a fallback for the spinner stuff but I understand the prompt is more complicated. I basically didn't want to assume we would get ansicon (or equivalent) into place before release and it kind of gives a "didn't test on windows" vibe to users to have the control sequences bleeding into the output...

But if we want to just commit to control sequence support on Windows sooner rather than later, I'm fine skipping any fallback for now in favor of ansicon work or whatever.

@DavidS
Copy link
Contributor

DavidS commented Jun 29, 2017

I need to do the sprint demo tonight, and would love to ship a new code drop before my PTO next week, so I'll hack on this, and the ansicon stuff today.

@bmjen bmjen merged commit fc5cafd into puppetlabs:master Jun 29, 2017
@rodjek rodjek deleted the sdk-284 branch November 26, 2018 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants