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

feat(up): add --allow-transient-write #618

Merged
merged 1 commit into from
Jul 14, 2022
Merged

feat(up): add --allow-transient-write #618

merged 1 commit into from
Jul 14, 2022

Conversation

FrankYang0529
Copy link
Contributor

@FrankYang0529 FrankYang0529 commented Jul 7, 2022 •
edited
Loading

resolve #605

Test cases

Please run the following script at the end of each case.

for entry in $TEMP/assets/**/*.txt
do
  test -w $entry && echo "Writable" || echo "Not Writable"
done

Local

Setup

cargo build
cd tests/http/simple-spin-rust

Case 1: new file can be set to read-only
Should show Not Writable.

TEMP=$(mktemp -d)
../../../target/debug/spin up --temp $TEMP

Case 2: same temporary folder can be changed to writable
Follow case 1. Should show Writable.

../../../target/debug/spin up --temp $TEMP --allow-transient-write

Case 3: new file can be set to writable
Should show Writable.

TEMP=$(mktemp -d)
../../../target/debug/spin up --temp $TEMP --allow-transient-write

Bindle

Setup

BINDLE_TEMP=$(mktemp -d)
bindle-server --unauthenticated -d $BINDLE_TEMP
cd tests/http/simple-spin-rust
../../../target/debug/spin bindle push

Case 4: new file can be set to read-only
Should show Not Writable.

TEMP=$(mktemp -d)
./target/debug/spin up --temp $TEMP --bindle spin-hello-world/1.0.0

Case 5: same temporary folder content can be updated
Stop the bindle-server and start a new bindle-server with a new temporary folder.

BINDLE_TEMP=$(mktemp -d)
bindle-server --unauthenticated -d $BINDLE_TEMP

Update content in spin/tests/http/simple-spin-rust/assets/test.txt. Run spin up again. The file in the temporary folder should be updated and it is still Not Writable after the update.

cd tests/http/simple-spin-rust
echo "test" > assets/test.txt
../../../target/debug/spin bindle push
cd ../../../
./target/debug/spin up --temp $TEMP --bindle spin-hello-world/1.0.0

Case 6: same temporary folder can be changed to writable
Follow case 5. Should show Writable.

./target/debug/spin up --temp $TEMP --bindle spin-hello-world/1.0.0 --allow-transient-write

Case 7: new file can be set to writable
Should show Not Writable.

TEMP=$(mktemp -d)
./target/debug/spin up --temp $TEMP --bindle spin-hello-world/1.0.0 --allow-transient-write

itowlson
itowlson reviewed Jul 8, 2022
View reviewed changes
crates/loader/src/local/assets.rs Outdated
let perms = tokio::fs::metadata(&to).await?.permissions();
// perms.set_readonly(true);
tokio::fs::set_permissions(&to, perms).await?;
let mut perms = tokio::fs::metadata(&to).await?.permissions();
Copy link
Contributor

Choose a reason for hiding this comment

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

The let mut perms = ...; perms.set_readonly(...); set_permissions(..., perms).await.with_context(...) sequence seems to happen in quite a few places. Could we create a utility function (e.g. set_writeable or allow_write) to tidy and reveal intent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I add change_file_permission function for it. Thank you.

@vdice vdice requested a review from itowlson July 11, 2022 21:07
itowlson
itowlson requested changes Jul 11, 2022
View reviewed changes
Copy link
Contributor

@itowlson itowlson 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 to me - thanks! Just two comments - the doc string is must-fix, but the comment one is just nice to have.

crates/loader/src/bindle/assets.rs Outdated
return Ok(());
}
Ok(false) => {
// if destination file is read-only, set it to writable first
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment sent me in the wrong direction a bit (I thought there was a bug where you forgot to consult allow_transient_writes), partly because check_existing_file is already confusing! I think this branch is "file exists but digest doesn't match so it's like corrupted. In which case this is "set it writeable because we need to overwrite it" I think? Sort of a "comment says what the code is doing but needs to say why instead" issue if that makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I change it to file exists but digest doesn't match, set it to writable first for further writing. Do you think it's more clear? Thank you.

src/commands/up.rs Outdated
@@ -81,6 +81,10 @@ pub struct UpCommand {
#[clap(long = "temp")]
pub tmp: Option<PathBuf>,

/// Temporary directory for the static assets of the components.
Copy link
Contributor

Choose a reason for hiding this comment

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

This doc string doesn't look right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it. Thank you.

itowlson
itowlson approved these changes Jul 13, 2022
View reviewed changes
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

This looks good to me - thanks for the tidying! There is a doc string that I missed before (sorry) that I think is wrong but it's not user-facing so it's not a blocker - but if you have time to fix it then that would be awesome. Otherwise happy to merge and fix it up later.

crates/loader/src/assets.rs Outdated
@@ -90,6 +90,19 @@ pub fn file_sha256_digest_string(path: impl AsRef<Path>) -> std::io::Result<Stri
Ok(digest_string)
}

/// Return an error if a path is not under a given directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies for missing this before but I think this doc string is incorrect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @itowlson, no worries, it's my fault that I didn't check it carefully. Thanks for catching it!

@FrankYang0529
feat(up): add --allow-transient-write
e696996 
Signed-off-by: Frank Yang <yangpoan@gmail.com>
itowlson
itowlson approved these changes Jul 14, 2022
View reviewed changes
Copy link
Contributor

@itowlson itowlson 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 - thanks and sorry for the multiple go-arounds!

@itowlson itowlson merged commit 785a40a into fermyon:main Jul 14, 2022
@FrankYang0529 FrankYang0529 deleted the files-default-read-only branch July 15, 2022 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itowlson itowlson itowlson approved these changes

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

Successfully merging this pull request may close these issues.

Make files mounts read-only by default
2 participants
@FrankYang0529 @itowlson