1128 - Print the entropy to stderr regardless of quiet mode

[eacherkan-aternity]

Judging by the prompter.StringRequired("Enter verification code") usage in the same method, I assume that it's reasonable to print to stderr if user input is required, even in quiet mode.

The logger's destination is set in main.go, either to io.Discard (in quiet mode), or to os.Stderr otherwise. Since at the moment there's no great flexibility in the logger configuration, I propose a simple solution by printing the specific line Phone approval required. Entropy is: %d always to stderr. I implement this by saving the previous destination aside, and putting it back after the Printf.

Potential downsides of this solution include:

• If at some point complex logger configuration is required, this will be one more place to fix.
• The UX is different from interactions that require user input, which go through the prompter. However, the prompter framework doesn't seem to have facilities for an informative user message that doesn't require input.

[codecov-commenter]

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (bc68124) 39.24% compared to head (af044ed) 39.27%.

Files	Patch %	Lines
pkg/prompter/prompter.go	0.00%	2 Missing ⚠️
pkg/prompter/survey.go	0.00%	2 Missing ⚠️
pkg/provider/aad/aad.go	50.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1129      +/-   ##
==========================================
+ Coverage   39.24%   39.27%   +0.03%     
==========================================
  Files          53       53              
  Lines        8022     8028       +6     
==========================================
+ Hits         3148     3153       +5     
  Misses       4452     4452              
- Partials      422      423       +1     

Flag	Coverage Δ
unittests	39.27% <37.50%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mapkon]

Hey @eacherkan-aternity

Our code coverage has gone down. Can you write a test that exercises the else branch?

[eacherkan-aternity]

Hi @mapkon, I added a unit test per your request.

[eacherkan-aternity]

I took the liberty of merging the recent changes from master to the branch. Looking at last night's build failure, I don't understand why it failed - the only failed tests seem to come from okta_test, unrelated to this branch's changes. All tests pass successfully on my machine.

[mapkon]

I took the liberty of merging the recent changes from master to the branch. Looking at last night's build failure, I don't understand why it failed - the only failed tests seem to come from okta_test, unrelated to this branch's changes. All tests pass successfully on my machine.

I sometimes get these intermittent failures -

[eacherkan-aternity]

@mapkon What are the next steps? Is anything required from my side?

[mapkon]

CC: @wolfeidau @gliptak

[gliptak]

thank your for the detailed analysis @eacherkan-aternity

while survey doesn't seem to have native support for stderr/always display, consider moving the stderr switching functionality into prompter

PS #1147

[mapkon]

thank your for the detailed analysis @eacherkan-aternity

while survey doesn't seem to have native support for stderr/always display, consider moving the stderr switching functionality into prompter

PS #1147

@eacherkan-aternity Any response to this?

[eacherkan-aternity]

@mapkon I think the decision whether the currently proposed solution is reasonable or whether the functionality should go into prompter should be yours (the maintainers'). As a first-time contributor I don't feel familiar enough with the project to make a design decision.

If you reject the current proposal (which of course is absolutely fine, no hard feelings whatsoever), I can try to move the functionality into prompter. As far as I understand, this entails:

1. Adding a new function for output-only (called Print? Display?) to the Prompter interface,
2. Implementing it in CliPrompter and PinentryPrompter,
3. Replacing some of the calls to log.Print* across the codebase with the new function,
4. Adding unit tests.

Does that make sense?

[gliptak]

thank you for detailing approach above. is there a need for separate PinentryPrompter or would CliPrompter gain additional methods? consider formulating above as a separate PR (and we could rebase this PR on it)

[mapkon]

@mapkon I think the decision whether the currently proposed solution is reasonable or whether the functionality should go into prompter should be yours (the maintainers'). As a first-time contributor I don't feel familiar enough with the project to make a design decision.

If you reject the current proposal (which of course is absolutely fine, no hard feelings whatsoever), I can try to move the functionality into prompter. As far as I understand, this entails:

1. Adding a new function for output-only (called Print? Display?) to the Prompter interface,
2. Implementing it in CliPrompter and PinentryPrompter,
3. Replacing some of the calls to log.Print* across the codebase with the new function,
4. Adding unit tests.

Does that make sense?

Thanks mate for the submission, and I appreciate your work on this, and contribution to open source.

To be fair, I maintain the project mainly by testing dependency updates and merging PRs. For the heavy lifting stuff, I defer to @gliptak and @wolfeidau for guidance, since they have more context than I do.

I would take their word over mine.

[mapkon]

@mapkon I think the decision whether the currently proposed solution is reasonable or whether the functionality should go into prompter should be yours (the maintainers'). As a first-time contributor I don't feel familiar enough with the project to make a design decision.

If you reject the current proposal (which of course is absolutely fine, no hard feelings whatsoever), I can try to move the functionality into prompter. As far as I understand, this entails:

1. Adding a new function for output-only (called Print? Display?) to the Prompter interface,
2. Implementing it in CliPrompter and PinentryPrompter,
3. Replacing some of the calls to log.Print* across the codebase with the new function,
4. Adding unit tests.

Does that make sense?

@eacherkan-aternity Any update on this?

[eacherkan-aternity]

@mapkon I'm working on the implementation.

is there a need for separate PinentryPrompter or would CliPrompter gain additional methods?

I don't see a need for a separate prompter, I would add a method to CliPrompter.

[mapkon]

@mapkon I'm working on the implementation.

is there a need for separate PinentryPrompter or would CliPrompter gain additional methods?

I don't see a need for a separate prompter, I would add a method to CliPrompter.

If that is easier to implement, lets go for that

[eacherkan-aternity]

Hi,
I implemented the change in Prompter and adjusted the tests accordingly.

[gliptak]

👍

[mapkon]

The linter is failing. Please take a look

[eacherkan-aternity]

Sorry. Fixed by running gofmt and goimports. Verified that golangci-lint runs without errors on my machine.