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 / update fstab entry with --update-fstab #2462

Merged
merged 6 commits into from
Aug 18, 2022
Merged

Conversation

timfeirg
Copy link
Contributor

@timfeirg timfeirg commented Aug 4, 2022

  • abs path check for sqlite and similars
  • correctly handle slice arguments, even though they don't exist in juicefs mount yet
  • do not run in containers
  • unit tests

PR code will produce the following result:

image

permission denied behavior:

image

closes #2432

@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2022

Codecov Report

Merging #2462 (97f35f2) into main (b1e5ebf) will decrease coverage by 1.78%.
The diff coverage is 53.19%.

❗ Current head 97f35f2 differs from pull request most recent head f77b909. Consider uploading reports for the commit f77b909 to get more accurate results

@@            Coverage Diff             @@
##             main    #2462      +/-   ##
==========================================
- Coverage   61.52%   59.73%   -1.79%     
==========================================
  Files         138      138              
  Lines       25698    23333    -2365     
==========================================
- Hits        15810    13938    -1872     
+ Misses       8030     7583     -447     
+ Partials     1858     1812      -46     
Impacted Files Coverage Δ
cmd/main.go 45.08% <50.00%> (-2.29%) ⬇️
cmd/mount.go 59.15% <51.13%> (-2.58%) ⬇️
cmd/mount_unix.go 47.22% <100.00%> (-0.53%) ⬇️
pkg/sync/config.go 0.00% <0.00%> (-83.34%) ⬇️
pkg/object/minio.go 61.29% <0.00%> (-15.19%) ⬇️
cmd/sync.go 56.46% <0.00%> (-12.87%) ⬇️
pkg/chunk/utils_linux.go 66.66% <0.00%> (-8.34%) ⬇️
cmd/version.go 42.85% <0.00%> (-7.15%) ⬇️
pkg/utils/utils.go 69.81% <0.00%> (-6.76%) ⬇️
pkg/object/file.go 65.76% <0.00%> (-6.71%) ⬇️
... and 130 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@timfeirg timfeirg force-pushed the feature/update-fstab branch from c03399c to 7c03731 Compare August 10, 2022 09:40
cmd/mount_unix.go Outdated Show resolved Hide resolved
cmd/mount.go Outdated Show resolved Hide resolved
cmd/mount.go Outdated Show resolved Hide resolved
cmd/mount.go Outdated Show resolved Hide resolved
cmd/mount.go Show resolved Hide resolved
cmd/mount.go Outdated Show resolved Hide resolved
@davies
Copy link
Contributor

davies commented Aug 11, 2022

Could you also update the docs (command ref and how to mount on boot)

Copy link
Contributor

@davies davies left a comment

Choose a reason for hiding this comment

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

this should not fail the mount

@timfeirg
Copy link
Contributor Author

I usually submit docs update after functionality has been confirmed, I'll work on them now.

@timfeirg
Copy link
Contributor Author

I wish to resolve #2504 first, and work on docs for this feature later on, in a separate PR.

cmd/mount_test.go Outdated Show resolved Hide resolved
cmd/mount.go Outdated Show resolved Hide resolved
@timfeirg timfeirg force-pushed the feature/update-fstab branch from 7d11d4d to 86cf44d Compare August 11, 2022 08:42
@timfeirg timfeirg force-pushed the feature/update-fstab branch from e6cebb0 to 6f0525a Compare August 11, 2022 09:38
@timfeirg timfeirg marked this pull request as draft August 11, 2022 09:38
@timfeirg
Copy link
Contributor Author

failed tests passed without any code change, if things go poorly in the future, remember that current state is able to PASS.

working on docs now, no code change will be introduced.

@davies davies force-pushed the feature/update-fstab branch from 6f0525a to e6cebb0 Compare August 11, 2022 13:14
@timfeirg timfeirg marked this pull request as ready for review August 11, 2022 15:44
cmd/mount.go Show resolved Hide resolved
cmd/mount.go Show resolved Hide resolved
@timfeirg
Copy link
Contributor Author

code change has been introduced since review, take another look at the follow-up commits? @davies


```shell
sudo cp juice/juicefs /sbin/mount.juicefs
可以用 `juiefs mount --update-fstab` 直接设置出自动挂载,例如:
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be available in 1.1 release, but user still read the latest docs, so we should add docs for new feature without replacing the old ways.

For these tutorials, we don't need to change them (keep them simple).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the old ways are preserved in guide/mount_at_boot.md, I can add minimum version requirements here (and remove after 1.1 has been widely adopted), would this suffice?

I think putting "the old way" here might just be too much content, after all, there's a dedicated chapter on this topic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For these tutorials, we don't need to change them (keep them simple).

--update-fstab is the simplest way to approach auto-mount, shouldn't we recommend its usage in all tutorials? if version inconsistency is the main issue here, can I postpone this docs update until after 1.1 release?


## Linux

Copy `juicefs` as `/sbin/mount.juicefs`, then edit `/etc/fstab` with following line:
Using `--update-fstab` flag with the mount command should automatically set things up for you:
Copy link
Contributor

Choose a reason for hiding this comment

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

This requires 1.1+

Copy link
Contributor

Choose a reason for hiding this comment

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

Since 1.1 is not released, we should put this new feature at the end of this section.

@timfeirg
Copy link
Contributor Author

can I create a separate PR for docs update? review suggestions so far are all concerning release planning & version inconsistency, which won't be an issue if we update docs only after 1.1 release, am I correct? @davies

@davies
Copy link
Contributor

davies commented Aug 18, 2022

If you prefer to have another PR, please revert the changes in docs, and we can merge the feature first.

@timfeirg timfeirg force-pushed the feature/update-fstab branch from 758b57c to 97f35f2 Compare August 18, 2022 06:58
@timfeirg
Copy link
Contributor Author

docs part has been moved to #2554

@davies davies merged commit d5b7e64 into main Aug 18, 2022
@davies davies deleted the feature/update-fstab branch August 18, 2022 07:24
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.

Add the automatic mount script
4 participants