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

Adds #42 CPU printout #55

Merged
merged 7 commits into from
Apr 16, 2024
Merged

Adds #42 CPU printout #55

merged 7 commits into from
Apr 16, 2024

Conversation

rustydb
Copy link
Contributor

@rustydb rustydb commented Mar 12, 2024

Summary and Scope

Issue Type

  • RFE Pull Request

Adds a new subcommand for printing out CPU information.

Additional changes:

  • Breaks up the MapPrint function into separate, reusable functions in a new print.go file.
  • Updates copyright headers

Risks and Mitigations

@rustydb rustydb requested a review from a team as a code owner March 12, 2024 19:49
@rustydb rustydb requested review from muresan, manderson-hpe, jpdavis-prof and jacobsalmela and removed request for a team, muresan, manderson-hpe and jpdavis-prof March 12, 2024 19:49
jacobsalmela
jacobsalmela requested changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@jacobsalmela jacobsalmela left a comment

Choose a reason for hiding this comment

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

Looks nice. Works well against real targets I tried as well as the simulator.

I think you should add some basic shellspec tests for these as well. They will be worth the effort.

@rustydb
Fix spec target
1d4e1f8 
- `spec` fails if the `spec/` directory exists, it needs to be declared
  as a `PHONY` or renamed (or both).
- The commands for installing `shellspec` will fail on MacOS. Modify
  the instructions to key off of the running OS, installing it
  appropriately.
- Rename `spec` to `shellspec-deps` to be more clear that this is
  just installing something
- Updates the usage for the `Makefile`, includes the test and simulator items.
- Updates the `.PHONY` list for all of the simulator items.
@rustydb
Remove comment
83138b9
@rustydb
Ignore the simulator
4af4f58
@rustydb
Add tests for show proc
05eed7b
jacobsalmela
jacobsalmela approved these changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@jacobsalmela jacobsalmela left a comment

Choose a reason for hiding this comment

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

Could add one more test for validating JSON output. Not holding this up though. It works nice in my testing.

@rustydb rustydb merged commit 33324d7 into main Apr 16, 2024
3 checks passed
@rustydb rustydb deleted the 42-cpu branch April 16, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobsalmela jacobsalmela jacobsalmela approved these changes

@jpdavis-prof jpdavis-prof Awaiting requested review from jpdavis-prof

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Include CPU architecture in system print out (for every CPU)
2 participants
@rustydb @jacobsalmela