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

fix(pam): Start broker and user selection only if required, without touching terminal #316

Merged

Conversation

3v1n0
Copy link
Collaborator

@3v1n0 3v1n0 commented Apr 16, 2024

Ensure that terminal output is not touched unless we really need to.

This is something that is also required for #314 so having it earlier would help that PR too.

UDENG-2608

…tion should be done

When this happens we should switch to the broker selection model
@3v1n0 3v1n0 force-pushed the start-broker-selection-only-if-required branch 2 times, most recently from 6e79d00 to b975910 Compare April 16, 2024 22:23
@codecov-commenter
Copy link

codecov-commenter commented Apr 16, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 84.54%. Comparing base (41e0b51) to head (5077953).

Files Patch % Lines
pam/internal/adapter/brokerselection.go 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #316      +/-   ##
==========================================
+ Coverage   84.51%   84.54%   +0.02%     
==========================================
  Files          82       82              
  Lines        6227     6230       +3     
  Branches       79       79              
==========================================
+ Hits         5263     5267       +4     
+ Misses        671      670       -1     
  Partials      293      293              

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

Delay broker selection until something requires it, so that we don't
need to write anything to the terminal if there's no need to select the
broker
…d one

In case we fail parsing the output better to consider it completely than
to provide an empty output as the golden file
So we avoid writing at all in the terminal if user selection is not
required, add tests to verify this behavior
@3v1n0 3v1n0 force-pushed the start-broker-selection-only-if-required branch from b975910 to 5077953 Compare April 16, 2024 22:30
@3v1n0 3v1n0 marked this pull request as ready for review April 16, 2024 22:34
@3v1n0 3v1n0 requested a review from a team as a code owner April 16, 2024 22:34
Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a small comment but it's not blocking.

examplebroker/broker.go Show resolved Hide resolved
@denisonbarbosa denisonbarbosa merged commit 4daf3d3 into ubuntu:main Apr 18, 2024
5 checks passed
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.

3 participants