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

Add manpages #8

Merged
merged 6 commits into from Jun 5, 2023
Merged

Add manpages #8

merged 6 commits into from Jun 5, 2023

Conversation

gliargovas
Copy link
Collaborator

No description provided.

@gliargovas
Copy link
Collaborator Author

@mgree @angelhof I have not added an example yet. Any recommendations?

Copy link
Contributor

@mgree mgree left a comment

Choose a reason for hiding this comment

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

looks good! just a few tiny tweaks

docs/try.1.md Outdated Show resolved Hide resolved
docs/try.1.md Outdated Show resolved Hide resolved
docs/try.1.md Outdated Show resolved Hide resolved
docs/try.1.md Outdated

# EXAMPLES

TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a few:

  1. An interactive example of a command that has moderately dispersed effect. Since apt doesn't work yet, maybe it could be un-taring a tarball with absolute paths?
  2. A non-interactive example that uses summary and commit. Maybe somehow doing the same work.
  3. A non-interactive example using the -D flag.

Copy link
Member

Choose a reason for hiding this comment

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

A make; make install could also be a potentially good candidate example (in the manpages and in general) or even a or pip install --user libdash.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

By default we block network operations inside the overlay. Maybe installing a local package using the .whl format?

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 that try by default should allow network operations (what do you think @mgree?) with a disclaimer that they will not be contained. Maybe we can add a flag.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to run pip install and got a timeout. wget and curl didn't work too.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. I think that this should not be the case. Could you open an issue with the examples that you run and what output you got to track the problem?

@gliargovas
Copy link
Collaborator Author

I have added some of the examples you proposed. Let me know if you would like me to add anything else :-)

Copy link
Member

@angelhof angelhof left a comment

Choose a reason for hiding this comment

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

Looks good other than the tiny comment I left

docs/try.1.md Outdated
Sometimes, you might want to pre-execute a command and commit its result at a later time.

```
try -D try_gunzip amendments.txt.gz
Copy link
Member

Choose a reason for hiding this comment

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

Should't this be try -D try_dir gunzip amendments.txt.gz based on the later try summary try_dir?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a typo, I will fix it

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you mean try -n gunzip amendments.txt.gz? that allocates a fresh directory but just gives the sandbox dir when it's done

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, makes sense. I didn't realize that -n gives you back the overlay dir

@angelhof
Copy link
Member

BTW, let's also add the example to the README too (once it's finalized)

@mgree
Copy link
Contributor

mgree commented May 30, 2023 via email

@gliargovas
Copy link
Collaborator Author

@angelhof @mgree I think this is ready to merge.

@angelhof
Copy link
Member

angelhof commented Jun 1, 2023

Looks good to me, just remember to document the issue that you had with pip, wget, and curl.

I tried to run pip install and got a timeout. wget and curl didn't work too.

@angelhof
Copy link
Member

angelhof commented Jun 1, 2023

Also, I think it is really important to add a test with whatever functionality we have in the manpage. The manpage example should always work and tested in CI. @gliargovas once you add the test, we can very easily make it part of CI (see https://github.com/binpash/sh-expand/blob/main/.github/workflows/test.yaml).

@gliargovas
Copy link
Collaborator Author

Looks good to me, just remember to document the issue that you had with pip, wget, and curl.

I tried to run pip install and got a timeout. wget and curl didn't work too.

I will open a separate issue documenting these.

@gliargovas
Copy link
Collaborator Author

Also, I think it is really important to add a test with whatever functionality we have in the manpage. The manpage example should always work and tested in CI. @gliargovas once you add the test, we can very easily make it part of CI (see https://github.com/binpash/sh-expand/blob/main/.github/workflows/test.yaml).

Makes sense, I will do so right now.

@angelhof angelhof merged commit 7f851e7 into binpash:main Jun 5, 2023
@angelhof angelhof mentioned this pull request Jun 5, 2023
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.

None yet

3 participants