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

Make files writable when they are exported. #149

Merged
merged 3 commits into from
Jul 8, 2024
Merged

Make files writable when they are exported. #149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
add8d26
Make files writable when they are exported.
plietar Jul 2, 2024
ba7a05c
bump version
plietar Jul 8, 2024
d872996
Add tests for modifying a dependency.
plietar Jul 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Package: orderly2
Title: Orderly Next Generation
Version: 1.99.19
Version: 1.99.20
Authors@R: c(person("Rich", "FitzJohn", role = c("aut", "cre"),
email = "[email protected]"),
person("Robert", "Ashton", role = "aut"),
Expand Down
5 changes: 5 additions & 0 deletions R/outpack_root.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ file_export <- function(root, id, there, here, dest, overwrite, call = NULL) {
}
fs::file_copy(there_full, here_full, overwrite)
}

# Files in the archive and file store are (generally) read-only.
# When exporting for interactive use, it's easier on the user if we make them
# writable again.
fs::file_chmod(here_full, "u+w")
}


Expand Down
30 changes: 30 additions & 0 deletions tests/testthat/test-outpack-helpers.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,3 +157,33 @@ test_that("good error message if file not found in packet", {
i = "For 'd.txt' did you mean 'a.txt', 'b.txt' or 'c.txt'",
i = "Remember that all orderly paths are case sensitive"))
})


test_that("Can overwrite when copying files from packet", {
Copy link
Member

Choose a reason for hiding this comment

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

can you add a test to verify that orderly_dependency is still copying files over read-only within a packet run? It's not immediately obvious to me from the implementation change that this will the the case. TBH I've entirely forgotten how the various bits of file exporting play together so it would be good to prevent any regression if we change the implementation

Copy link
Member

Choose a reason for hiding this comment

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

ok, looks like this is due to

  outpack_packet_file_mark(packet, result$files$here, "immutable")

in the active context, so that's nice

Copy link
Member Author

@plietar plietar Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

file_mark only adds a check the file's hash at the end of the packet run, it doesn't pro-actively prevent files from being modified (albeit the result is basically the same I suppose).

file_export, which I've modified here, does not get called by the orderly_run flow. I'll add a test still.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, I was wrong about that file_export not being called. With this PR, files in the draft folder become mutable again (as they were before #141).

The outpack_packet_file_mark still does it's job of checking at the end of the packet, so in my opinion that's fine. I've added an explicit test for this.

root <- create_temporary_root(use_file_store = TRUE)

id1 <- create_random_packet(root)
id2 <- create_random_packet(root)

# Just a bit of a sanity check. The two packets are random, so we'd expect
# them to have different contents.
expect_false(identical(
readRDS(file.path(root$path, "archive", "data", id1, "data.rds")),
readRDS(file.path(root$path, "archive", "data", id2, "data.rds"))))

dst <- temp_file()

suppressMessages(
orderly_copy_files(id1, files = "data.rds", dest = dst, root = root))

expect_identical(
readRDS(file.path(dst, "data.rds")),
readRDS(file.path(root$path, "archive", "data", id1, "data.rds")))

suppressMessages(
orderly_copy_files(id2, files = "data.rds", dest = dst, root = root))

expect_identical(
readRDS(file.path(dst, "data.rds")),
readRDS(file.path(root$path, "archive", "data", id2, "data.rds")))
})
17 changes: 17 additions & 0 deletions tests/testthat/test-outpack-packet.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,23 @@ test_that("Re-adding files triggers hash", {
})


test_that("Can detect changes to dependencies", {
root <- create_temporary_root()
id <- create_random_packet(root, "data")

path_src <- withr::local_tempdir()
p <- outpack_packet_start_quietly(path_src, "next", root = root)

outpack_packet_use_dependency(p, id, "data.rds")
saveRDS(1:10, file.path(path_src, "data.rds"))

err <- expect_error(
outpack_packet_end_quietly(p),
"File was changed after being added")
expect_equal(err$body, c(x = "data.rds"))
})


test_that("Can ignore files from the final packet", {
root <- create_temporary_root(path_archive = "archive", use_file_store = TRUE)
path_src <- create_temporary_simple_src()
Expand Down
Loading