Allow non-master branches #100
Conversation
Codecov Report
@@ Coverage Diff @@
## master #100 +/- ##
=======================================
Coverage 99.35% 99.36%
=======================================
Files 12 12
Lines 1083 1095 +12
=======================================
+ Hits 1076 1088 +12
Misses 7 7
Continue to review full report at Codecov.
|
r-ash
left a comment
r-ash
left a comment
There was a problem hiding this comment.
Looks great, couple of small things I noticed but changes look good
| api$handle(endpoint_git_commits(path)) | ||
| api$handle(endpoint_available_reports(path)) | ||
| api$handle(endpoint_report_parameters(path)) | ||
| api$handle(endpoint_git_status(runner)) |
There was a problem hiding this comment.
Can we remove path from build_api signature too now it is unused?
R/main.R
Outdated
| Options: | ||
| --port=PORT Port to run on [default: 8321] | ||
| --host=HOST IP address owned by this server [default: 0.0.0.0] | ||
| --no-ref Prevent git reference switching |
There was a problem hiding this comment.
Can remove this from docs too now usage has gone?
| test_that("git_pull", { | ||
| path <- orderly_prepare_orderly_git_example() | ||
| runner <- mock_runner(root = path[["local"]]) | ||
| endpoint <- endpoint_git_pull(runner) | ||
| res <- endpoint$run() | ||
| expect_equal(res$status_code, 200) | ||
| expect_equal(res$data, c("From upstream", | ||
| " 0fc0d08..0ec7621 master -> origin/master", | ||
| "Updating 0fc0d08..0ec7621", | ||
| "Fast-forward", " new | 1 +", | ||
| " 1 file changed, 1 insertion(+)", | ||
| " create mode 100644 new")) | ||
| mockery::expect_args(mock_pull, 1, "test_path") | ||
| expect_match(res$data, "Fast-forward", all = FALSE) | ||
| expect_equal( | ||
| git_ref_to_sha("HEAD", path[["local"]]), | ||
| git_ref_to_sha("HEAD", path[["origin"]])) |
There was a problem hiding this comment.
Nice, this looks like a better test!
R/runner.R
Outdated
| ##' @param identity The name of the server identity, as listed in | ||
| ##' orderly_config.yml's remote section | ||
| ##' | ||
| ##' Allow git to change branches/ref for run. If not | ||
| ##' given, then we will look to see if the orderly configuration | ||
| ##' disallows branch changes (based on the | ||
| ##' \code{ORDERLY_API_SERVER_IDENTITY} environment variable and the | ||
| ##' \code{master_only} setting of the relevant server block. | ||
| ##' \code{default_branch_only} setting of the relevant server block. |
There was a problem hiding this comment.
This is a little confusing, how about something like
@param identity The name of the server identity, as listed in orderly_config.yml's remote section. If not given, then we will fall back to using the \code{ORDERLY_API_SERVER_IDENTITY} environment variable. Used to set configuration specific to this server e.g. host, port, teams notification URL, default branch name
Or something similar
r-ash
left a comment
r-ash
left a comment
There was a problem hiding this comment.
Looks good to me. Generally though I am passing through path instead of the whole runner to some of the endpoints when those functions only need the path. I feel like passing the simplest thing through makes the functions interface simpler to understand. I think easier to look at endpoint with just a path and see what it is doing than taking a whole object. Also easier for testing because we don't have to create the object to run the test.
Branch pins should be removed after vimc/orderly1#318 is merged