-
Notifications
You must be signed in to change notification settings - Fork 77
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
Updating SkupperPodmanSite.Status to check if site.AuthMode is internal. #1453
base: main
Are you sure you want to change the base?
Conversation
Please see below for manual test results. Tests ran on fedora. Test case: configure console, --console-auth unsecured:
Test case: configure console with internal auth:
|
207b873
to
e56f82d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that there are two spaces after The site console url is:
(this is not related to your change, but it would be nice to have it fixed as well).
There is also an extra line being displayed at the end.
Do you mind removing it?
e56f82d
to
5d04831
Compare
Thanks for the review. Updated with the suggestions. Hope it's ok that I used fmt.Printf("...\n") instead of fmt.Println(). Re-ran test case by hand: configure console, --console-auth unsecured:
Re-ran test case by hand: configure console with internal auth:
|
Out of curiosity, I was wondering if this first line of
Just thought I'd double-check... Planning to leave as-is unless there's feedback. |
@Karen-Schoener thanks for your pull request!
At present it would affect all the tests that check the output of the skupper status, I would not recommend it for now. |
5d04831
to
e1e6a05
Compare
e1e6a05
to
80902cf
Compare
Rebased with the the latest in skupper/main. |
I believe that the review comments have been addressed. @fgiorgetti when time allows, would you mind letting me know if the PR looks ok? Thanks in advance, Karen |
Fixes #1389