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

cli: switch back to human readable version information #776

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

Freax13
Copy link
Contributor

@Freax13 Freax13 commented Jul 31, 2024

This PR changes the JSON dump back into a human readable version dump :)

@Freax13 Freax13 added the no changelog PRs not listed in the release notes label Jul 31, 2024
@Freax13 Freax13 requested review from burgerdev and msanft July 31, 2024 09:18
@Freax13 Freax13 requested a review from katexochen as a code owner July 31, 2024 09:18
@Freax13
Copy link
Contributor Author

Freax13 commented Jul 31, 2024

This PR depends on #772. The PR seemingly breaks CI, so I've manually cancelled the CI jobs for this PR. I'll restart them once the underlying issue is fixed.

@msanft
Copy link
Contributor

msanft commented Jul 31, 2024

Needs to wait for #775 too, I'm afraid

@msanft msanft force-pushed the msanft/runtime-handler-naming branch 5 times, most recently from 6e40ff8 to 80685d3 Compare August 1, 2024 15:27
@Freax13 Freax13 force-pushed the tom/pretty-version branch from aaef24e to 131e659 Compare August 2, 2024 06:31
@msanft msanft force-pushed the msanft/runtime-handler-naming branch from 80685d3 to 81b98be Compare August 2, 2024 06:44
@msanft
Copy link
Contributor

msanft commented Aug 2, 2024

I think this needs a rebase

@Freax13 Freax13 force-pushed the tom/pretty-version branch from 131e659 to 3f7740c Compare August 2, 2024 06:47
cli/main.go Outdated
case platforms.AKSCloudHypervisorSNP:
fmt.Fprintf(versionsWriter, "\tlaunch digest:\t%s\n", embeddedReferenceValues.AKS.TrustedMeasurement.String())
fmt.Fprint(versionsWriter, "\tdefault SNP TCB:\t\n")
fmt.Fprintf(versionsWriter, "\t bootloader:\t%d\n", embeddedReferenceValues.AKS.SNP.MinimumTCB.BootloaderVersion.UInt8())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fmt.Fprintf(versionsWriter, "\t bootloader:\t%d\n", embeddedReferenceValues.AKS.SNP.MinimumTCB.BootloaderVersion.UInt8())
fmt.Fprintf(versionsWriter, "\t\tbootloader:\t%d\n", embeddedReferenceValues.AKS.SNP.MinimumTCB.BootloaderVersion.UInt8())

nit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tabwriter does weird things if we use another tab:
image

Copy link
Contributor

@msanft msanft Aug 2, 2024

Choose a reason for hiding this comment

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

Hm.. This seems like it's intended? (for aligning everything)

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of handcrafting this output, can we maybe switch to a structured format that has opinionated rendering but decent readability, e.g. YAML or TOML?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Fine for me, if it contains the needed info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, tabwriter is doing what's it's supposed to to, but the behavior just doesn't fit our use-case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine for me, if it contains the needed info.

The embedded reference values don't contain the genpolicy version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@burgerdev @katexochen are you fine with hand-crafting the ouput for now? Maybe once we only use a single genpolicy version, we can just print the reference values verbatim.

@Freax13 Freax13 requested a review from msanft August 2, 2024 08:38
@msanft msanft force-pushed the msanft/runtime-handler-naming branch 6 times, most recently from 2cc8939 to 0eb46f1 Compare August 2, 2024 12:29
Base automatically changed from msanft/runtime-handler-naming to main August 2, 2024 12:47
@msanft
Copy link
Contributor

msanft commented Aug 2, 2024

needs another rebase, sorry.

Changes LGTM though.

@Freax13 Freax13 force-pushed the tom/pretty-version branch from 3f7740c to 89f300b Compare August 12, 2024 08:05
cli/main.go Outdated Show resolved Hide resolved
@Freax13 Freax13 force-pushed the tom/pretty-version branch 2 times, most recently from 344c43a to e1ce818 Compare August 14, 2024 07:17
@Freax13 Freax13 force-pushed the tom/pretty-version branch from e1ce818 to b6cb256 Compare August 14, 2024 07:30
@Freax13 Freax13 requested a review from katexochen August 14, 2024 07:30
@Freax13 Freax13 merged commit ab16b23 into main Aug 14, 2024
9 checks passed
@Freax13 Freax13 deleted the tom/pretty-version branch August 14, 2024 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog PRs not listed in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants