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.

Sign up for GitHub

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

Jump to bottom

Improved "Network" url in cli #57882

Closed
wants to merge 3 commits into from
Closed

Improved "Network" url in cli #57882

wants to merge 3 commits into from
+48 −2

Conversation

kletse
Copy link
Contributor

@kletse kletse commented Nov 1, 2023 •
edited
Loading

What?

Fix an old feature request to show the current ip address in the cli

Why?

Improve DX

How?

Look at the actual available network interfaces and use the first relevant IP.
Picks an IPv4 or IPv6 hostname based on the hostname used to show the url for 'Local:', so both are of the same IP version.

fixes #11367
related to PR #22352

Example output:
image

@ijjk ijjk added the type: next label Nov 1, 2023
leerob
leerob requested changes Nov 8, 2023
View reviewed changes
Copy link
Member

@leerob leerob left a comment

Choose a reason for hiding this comment

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

Could you add tests please?

@kletse
Copy link
Contributor Author

kletse commented Nov 8, 2023

@leerob Should be doable to actually unit test the function I added. Give me some time to figure this out over the weekend. 😀

@BlankParticle
Copy link
Contributor

BlankParticle commented Jan 20, 2024 •
edited
Loading

This is a feature that would make my life so much easier.
Is there anyway I could help?

Could you add tests please?

I can't think of any way how the function in question can be tested as I have little to no experince in testing. Can you please tell how it should be tested, then I will try to add a test based on the requirement.

Thanks

@SrPatinhas
Copy link

@kletse were you able to add some tests? This would be a nice feature for the devs

@kletse
Copy link
Contributor Author

kletse commented Feb 1, 2024

Still want to pick this up again. Maybe this weekend, not sure. Family first 😅

@ijjk ijjk mentioned this pull request Nov 12, 2024
Merged
ijjk
ijjk approved these changes Nov 12, 2024
View reviewed changes
Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR! Sorry for the lag here, went ahead and added a test case for this in #72634 and got it up to date with canary

@ijjk ijjk closed this in #72634 Nov 12, 2024
ijjk added a commit that referenced this pull request Nov 12, 2024
@ijjk @kletse
improved network url in (dev) cli (#72634)
b888c42 
Continues: #57882 with added test
and rebased history

Closes: #57882

---------

Co-authored-by: Ward Werbrouck <34573943+kletse@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leerob leerob leerob requested changes

@ijjk ijjk ijjk approved these changes

@timneutkens timneutkens Awaiting requested review from timneutkens

@shuding shuding Awaiting requested review from shuding

@huozhi huozhi Awaiting requested review from huozhi

@feedthejim feedthejim Awaiting requested review from feedthejim

@ztanner ztanner Awaiting requested review from ztanner

@wyattjoh wyattjoh Awaiting requested review from wyattjoh

Assignees
No one assigned
Labels
type: next
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] View the app "On Your Network"
5 participants
@kletse @BlankParticle @SrPatinhas @leerob @ijjk