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

feat(cli): amtinfo command add amt ipaddr and osipaddr #560

Conversation

tongsean9807
Copy link
Contributor

Add os ipaddress to amtinfo command.
amtinfo command now show amt ipaddrss and os ipaddress.
os ipadress select by mapping to mac address from amt.

Resolves: #467

@tongsean9807
Copy link
Contributor Author

Output:
image

Copy link
Member

@rsdmike rsdmike left a comment

Choose a reason for hiding this comment

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

I like this being added. I left a few nit picks, but most importantly is to remove the uint32 conversion and move it out of the PTHI package and into the internal package, likely in the internal/local/info.go file.

pkg/pthi/commands.go Outdated Show resolved Hide resolved
pkg/pthi/commands.go Outdated Show resolved Hide resolved
pkg/pthi/commands.go Outdated Show resolved Hide resolved
pkg/pthi/commands.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jun 18, 2024

Codecov Report

Attention: Patch coverage is 71.69811% with 15 lines in your changes missing coverage. Please review.

Project coverage is 71.29%. Comparing base (650935d) to head (93ba577).
Report is 250 commits behind head on main.

Files Patch % Lines
internal/local/info.go 71.15% 10 Missing and 5 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #560       +/-   ##
===========================================
- Coverage   83.21%   71.29%   -11.92%     
===========================================
  Files          32       41        +9     
  Lines        4265     4132      -133     
===========================================
- Hits         3549     2946      -603     
- Misses        576      919      +343     
- Partials      140      267      +127     

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

pkg/pthi/commands.go Outdated Show resolved Hide resolved
@tongsean9807 tongsean9807 changed the title style(pthi): amtinfo command add amt ipaddr and osipaddr style(cli): amtinfo command add amt ipaddr and osipaddr Jun 19, 2024
@tongsean9807
Copy link
Contributor Author

Hi,

I have move the GetOSIPAddress() to internal/local/info.go
Let me know what other changes is needed.

I will squash them after fixing all the changes.

Thank you

@tongsean9807 tongsean9807 requested a review from rsdmike June 20, 2024 02:13
@rsdmike
Copy link
Member

rsdmike commented Jun 20, 2024

@tongsean9807 looks good to me. You think you can muster up a unit test or two? Additionally, the commit message should really be feat: since its a new capability added to the cli.

internal/local/info.go Outdated Show resolved Hide resolved
@tongsean9807 tongsean9807 changed the title style(cli): amtinfo command add amt ipaddr and osipaddr feat(cli): amtinfo command add amt ipaddr and osipaddr Jun 21, 2024
@tongsean9807
Copy link
Contributor Author

Hi,

I have changed the space to tab, which causes the misalignment when viewing in github.
I have added the unittest but I am unable to overwrite the net.interface.
Let me know if there are better ways to improve the unittest.

Thank you

@tongsean9807 tongsean9807 requested a review from rsdmike June 21, 2024 09:32
@rsdmike
Copy link
Member

rsdmike commented Jun 21, 2024

Hi @tongsean9807 , Thank you for adding the unit test! I should have sent this to you, we have a NetEnumerator

type NetEnumerator struct {
Interfaces func() ([]net.Interface, error)
that you can use to abstract the net.Interfaces calls.

Can take a look at

var testNetEnumerator = NetEnumerator{
for an example of how it is used in tests.

For the code that uses the NetEnumerator check out:

ifaces, err := f.netEnumerator.Interfaces()
.

@tongsean9807 tongsean9807 force-pushed the tongsean/amtinfo_add_amt_and_os_ip branch from f96152d to cd6a619 Compare June 23, 2024 07:28
@tongsean9807
Copy link
Contributor Author

Hi,

I have add positive and negative unit test.
And use the net enumerator from flags package.

Thank you for the pointing out the net enumerator.

Regards,
Sean

@tongsean9807 tongsean9807 force-pushed the tongsean/amtinfo_add_amt_and_os_ip branch from 6cc4299 to f6e6bcd Compare June 25, 2024 03:05
Add os ipaddress to amtinfo command.
amtinfo command now show amt ipaddress and os ipaddress.
os ipaddress select by mapping to mac address from amt.

Resolves: open-amt-cloud-toolkit#467
@tongsean9807 tongsean9807 force-pushed the tongsean/amtinfo_add_amt_and_os_ip branch from f6e6bcd to 4226e84 Compare June 25, 2024 03:38
@tongsean9807
Copy link
Contributor Author

Hi @rsdmike,

I hope you're doing well. I want to follow up on this PR.
I have made the changes.
Have you had a chance to review it?

Thank you

tongsean9807

This comment was marked as off-topic.

@rsdmike
Copy link
Member

rsdmike commented Jun 27, 2024

Hi @rsdmike,

I hope you're doing well. I want to follow up on this PR. I have made the changes. Have you had a chance to review it?

Thank you

Thanks for following up, looks good to me! We'll get it merged.

@rsdmike rsdmike merged commit 77b42aa into open-amt-cloud-toolkit:main Jun 27, 2024
12 of 14 checks passed
RosieAMT pushed a commit that referenced this pull request Jul 15, 2024
# [2.36.0](v2.35.0...v2.36.0) (2024-07-15)

### Features

* **cli:** amtinfo command add amt ipaddr and osipaddr ([#560](#560)) ([77b42aa](77b42aa)), closes [#467](#467)
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.

AMTinfo: Add additional OS and AMT network details to rpc.exe amtinfo
2 participants