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

Updated README.md #118

Closed
wants to merge 0 commits into from
Closed

Updated README.md #118

wants to merge 0 commits into from

Conversation

kairveeehh
Copy link
Contributor

solves (#96 )
Updated the readme to have separate copyable commands.

@kairveeehh kairveeehh force-pushed the master branch 4 times, most recently from c4b7b71 to 5834e50 Compare February 19, 2025 22:17
@kairveeehh
Copy link
Contributor Author

@nirs kindly review and merge.

Copy link
Member

@nirs nirs left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

README.md Outdated
curl -OSL "https://github.com/lima-vm/socket_vmnet/releases/download/${VERSION}/${FILE}"
```
Copy link
Member

Choose a reason for hiding this comment

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

I think we can improve the user experience a little bit by merging the getting the latest version and download steps to one script. We can add a sentence after the example explaining the details.

Download the latest release binary:

VERSION="$(curl -fsSL https://api.github.com/repos/lima-vm/socket_vmnet/releases/latest | jq -r .tag_name)"
FILE="socket_vmnet-${VERSION:1}-$(uname -m).tar.gz"
curl -OSL "https://github.com/lima-vm/socket_vmnet/releases/download/${VERSION}/${FILE}"

This gets the latest release version using github api and download the binary.

But let's wait for @jandubois review, he was concerned about removing the comments.

README.md Outdated
gh attestation verify --owner=lima-vm "${FILE}"
```
Copy link
Member

Choose a reason for hiding this comment

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

The problem with this part is that most people will skip this part since this is optional. I think we want to help people to start using attestations.

Also the text is not helpful because most user do not know that is "GitHub Artifact Attestation". We should replace this with a short sentence using term that most users already know, and link to relevant docs adding more info.

We can use an alert like this:

Tip

To verify the downloaded binary using GitHub's gh command (https://cli.github.com), run:

gh attestation verify --owner=lima-vm "${FILE}"

Learn more about GitHub Artifact Attestation

README.md Outdated
tar tzvf "${FILE}"
```
Copy link
Member

Choose a reason for hiding this comment

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

I think this we can drop this part. tar is well known and we don't need to explain that you can view the binary using it.

But let's wait for @AkihiroSuda ack on this.

@kairveeehh
Copy link
Contributor Author

kairveeehh commented Feb 20, 2025

alright awaiting both of their response on it. will do the fixes accordingly.

@nirs
Copy link
Member

nirs commented Feb 26, 2025

@kairveeehh closed by mistake?

@kairveeehh
Copy link
Contributor Author

@kairveeehh closed by mistake?

Yes actually this was causing trouble over the other PR I had to raise so closed this , and will reopen this one.
It was a mistake while trying to fix things up 😅

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.

2 participants