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

Restore changes from #90, "Refactor PromptUI" #120

Merged
merged 2 commits into from
Apr 4, 2023
Merged

Conversation

szh
Copy link
Contributor

@szh szh commented Mar 29, 2023

Bug report details

conjur-cli-go binary displays double prompt on windows terminal.
This may be related to prompt-ui refactor. Does not seem to impact functionality

Steps to reproduce:

  • Login to Windows system
  • open cmd terminal
  • run conjur init command

Current Results:
results in double prompt

Expected Results:
single prompt

Desired Outcome

Windows displays single prompt

Implemented Changes

Connected Issue/Story

CyberArk internal issue ID: CNJR-475

Definition of Done

  • Windows displays only single prompt

Changelog

  • The CHANGELOG has been updated, or
  • This PR does not include user-facing changes and doesn't require a
    CHANGELOG update

Test coverage

  • This PR includes new unit and integration tests to go with the code
    changes, or
  • The changes in this PR do not require tests

Documentation

  • Docs (e.g. READMEs) were updated in this PR
  • A follow-up issue to update official docs has been filed here: [insert issue ID]
  • This PR does not require updating any documentation

Behavior

  • This PR changes product behavior and has been reviewed by a PO, or
  • These changes are part of a larger initiative that will be reviewed later, or
  • No behavior was changed with this PR

Security

  • Security architect has reviewed the changes in this PR,
  • These changes are part of a larger initiative with a separate security review, or
  • There are no security aspects to these changes

@szh szh self-assigned this Mar 29, 2023
@szh szh force-pushed the prompts-refactor branch 2 times, most recently from eb9b078 to c1b6315 Compare March 29, 2023 13:35
@szh szh added the kind/bug label Mar 29, 2023
@szh szh force-pushed the prompts-refactor branch from c1b6315 to bc8a780 Compare March 29, 2023 15:08
@szh szh marked this pull request as ready for review March 29, 2023 20:47
@szh szh requested review from a team as code owners March 29, 2023 20:47
@szh szh marked this pull request as draft March 29, 2023 20:53
@szh szh force-pushed the prompts-refactor branch 2 times, most recently from e225fd8 to 4be26f3 Compare March 30, 2023 15:20
@szh szh force-pushed the prompts-refactor branch from 4be26f3 to fcc14a2 Compare March 30, 2023 15:32
@codeclimate
Copy link

codeclimate bot commented Mar 30, 2023

Code Climate has analyzed commit fcc14a2 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 71.4% (50% is the threshold).

This pull request will bring the total coverage in the repository to 81.4%.

View more on Code Climate.

@szh szh marked this pull request as ready for review March 30, 2023 15:45
Copy link
Contributor

@gl-johnson gl-johnson left a comment

Choose a reason for hiding this comment

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

Thanks for fixing up this branch! Nice workaround to address the unit test delays in the original PR. I also like that it allows us to clean up some of the utils and avoid passing the PromptDecorator func around.

I suggest we hold out for another reviewer to approve since I was heavily involved in the original PR.

Copy link
Contributor

@john-odonnell john-odonnell left a comment

Choose a reason for hiding this comment

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

LGTM - goodbye decoratePrompt & setCommandStreamsOnPrompt!

@szh szh merged commit cbb0d9e into master Apr 4, 2023
@szh szh deleted the prompts-refactor branch April 4, 2023 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants