Submission: circle

[pat-s]

Submitting Author: Patrick Schratz (@pat-s)
Repository: https://github.com/ropenscilabs/circle
Version submitted: v0.5.0.9001
Editor: @noamross
Reviewers: @sharlagelfand, @mbjoseph

Due date for @sharlagelfand: 2020-12-07

Due date for @mbjoseph: 2020-12-07
Archive: TBD
Version accepted: TBD

---

• Paste the full DESCRIPTION file inside a code block below:

Package: circle
Title: R Client Package for Circle CI
Version: 0.7.0
Authors@R:
    person(given = "Patrick",
           family = "Schratz",
           role = c("aut", "cre"),
           email = "patrick.schratz@gmail.com",
           comment = c(ORCID = "0000-0003-0748-6624"))
Description: Tools for interacting with the 'Circle CI' API.
    Besides executing common tasks such as querying build logs and
    restarting builds, this package also helps setting up permissions to
    deploy from builds.
License: GPL-3
URL: https://docs.ropensci.org/circle/,
    https://github.com/ropenscilabs/circle
BugReports: https://github.com/ropenscilabs/circle/issues
Imports:
    cli (>= 2.0.0),
    httr,
    jsonlite
Suggests:
    clipr,
    covr,
    gert,
    gh,
    knitr,
    openssl,
    purrr,
    rmarkdown,
    testthat (>= 3.0.0),
    usethis (>= 2.0.0),
    utils,
    vcr,
    withr
VignetteBuilder:
    knitr
Config/testthat/edition: 3
Config/testthat/parallel: false
Encoding: UTF-8
LazyData: true
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.1.1

Scope

• Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):

  • data retrieval
  • data extraction
  • database access
  • data munging
  • data deposition
  • workflow automation
  • version control
  • citation management and bibliometrics
  • scientific software wrappers
  • database software bindings
  • geospatial data
  • text analysis

• Explain how and why the package falls under these categories (briefly, 1-2 sentences):

It helps to automate CI workflows and goes in line with the {tic} and {travis} package.

• Who is the target audience and what are scientific applications of this package?

Users who prefer to interact with the command line to query information about CI build statuses.
No scientific applications.

• Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?

Besides a discontinued package which is 4 years old, there is no similar package.

• If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.

Technical checks

Confirm each of the following by checking the box. This package:

• does not violate the Terms of Service of any service it interacts with.
• has a CRAN and OSI accepted license.
• contains a README with instructions for installing the development version.
• includes documentation with examples for all functions.
• contains a vignette with examples of its essential functions and uses.
• has a test suite.
• has continuous integration, including reporting of test coverage using services such as Travis CI, Coveralls and/or CodeCov.

Publication options

• Do you intend for this package to go on CRAN?
• Do you wish to automatically submit to the Journal of Open Source Software? If so:

JOSS Options

• The package has an obvious research application according to JOSS's definition.
  • The package contains a paper.md matching JOSS's requirements with a high-level description in the package root or in inst/.
  • The package is deposited in a long-term repository with the DOI:
  • (Do not submit your package separately to JOSS)

• Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:

MEE Options

• The package is novel and will be of interest to the broad readership of the journal.
• The manuscript describing the package is no longer than 3000 words.
• You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see MEE's Policy on Publishing Code)
• (Scope: Do consider MEE's Aims and Scope for your manuscript. We make no guarantee that your manuscript will be within MEE scope.)
• (Although not required, we strongly recommend having a full manuscript prepared when you submit here.)
• (Please do not submit your package separately to Methods in Ecology and Evolution)

Code of conduct

• I agree to abide by rOpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.

[maelle]

Many thanks @pat-s for your submission! It is in scope indeed. However we think it'd make more sense to finalize #305 first as you're quite involved in that one as well.

[pat-s]

Thanks Maelle. {circle} and {travis} are both deps of {tic} to some degree but also kind of independent.

Let's finish {tic} first then to not confuse things.

[pat-s]

{tic} is done - ready when you are :)

[noamross]

Editor checks:

• Fit: The package meets criteria for fit and overlap
• Automated tests: Package has a testing suite and is tested via Travis-CI or another CI service.
• License: The package has a CRAN or OSI accepted license
• Repository: The repository link resolves correctly
• Archive (JOSS only, may be post-review): The repository DOI resolves correctly
• Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

---

Editor comments

Thanks for your submission, @pat-s! My initial attempt to test this package failed (see below). Based on Failed to creating checkout keys for repo pat-s/travis-testthat, and I still got failures when I forked the repository to my own GitHub account and set that as the only remote (I have a CircleCI account). With packages like this it's common for some permission issues to arise in this way. Before I assign to reviewers could you please document the developer workflow so reviewers know what remotes, accounts, keys etc., they need set up in order to test the package locally? This is best placed in CONTRIBUTING.md and linked from the README.

Also, to preempt another issue, your README is somewhat sparse and both it and the vignette lack an introduction to what the package is for and, importantly in this case, what it is not for. Remember our principle of multiple points of entry*: any piece of documentation may be the first encounter the user has with the package and/or the tool/data it wraps. It will be helpful to start these out with an introduction that explains that the package is for managing a CI service and its relationship to tic and other tools.

Loading circle
Testing circle
✓ |  OK F W S | Context
✓ |   1       | authentication [1.0 s]
x |   2 1     | builds [10.0 s]
──────────────────────────────────────────────────────────────────────────────
test-builds.R:24: error: restarting a workflow works
Forbidden (HTTP 403). Failed to restarting workflow 'c006f0a5-5c05-4306-b66f-dc04f141e2ae' on Circle CI.
Backtrace:
 1. testthat::expect_s3_class(retry_workflow(workflow_id), "circle_api") tests/testthat/test-builds.R:24:2
 4. circle::retry_workflow(workflow_id)
 5. httr::stop_for_status(...) R/builds.R:145:2
──────────────────────────────────────────────────────────────────────────────
x |   0 2     | checkout-key [0.2 s]
──────────────────────────────────────────────────────────────────────────────
test-checkout-keys.R:8: error: setting checkout keys works
Forbidden (HTTP 403). Failed to creating checkout keys for repo pat-s/travis-testthat on Circle CI.
Backtrace:
 1. testthat::expect_s3_class(...) tests/testthat/test-checkout-keys.R:8:2
 4. circle::create_checkout_key(repo = repo, user = user, type = "user-key")
 5. httr::stop_for_status(...) R/checkout-key.R:38:2

test-checkout-keys.R:18: error: deleting checkout keys works
Forbidden (HTTP 403). Failed to get checkout keys for repo pat-s/travis-testthat on Circle CI.
Backtrace:
 1. circle::get_checkout_keys(repo = repo, user = user) tests/testthat/test-checkout-keys.R:18:2
 2. httr::stop_for_status(...) R/checkout-key.R:67:2
──────────────────────────────────────────────────────────────────────────────
x |   0 3     | env-vars [0.2 s]
──────────────────────────────────────────────────────────────────────────────
test-env-vars.R:6: error: setting env vars works
Forbidden (HTTP 403). Failed to set env vars for repo 'pat-s/travis-testthat' on Circle CI.
Backtrace:
  1. testthat::expect_silent(...) tests/testthat/test-env-vars.R:6:2
  9. circle::set_env_var(...)
 10. httr::stop_for_status(...) R/env-var.R:96:2

test-env-vars.R:16: error: getting env vars works
Forbidden (HTTP 403). Failed to get env vars for repo pat-s/travis-testthat on Circle CI..
Backtrace:
  1. testthat::expect_silent(get_env_vars(repo = repo, user = user)) tests/testthat/test-env-vars.R:16:2
  9. circle::get_env_vars(repo = repo, user = user)
 10. httr::stop_for_status(...) R/env-var.R:51:2

test-env-vars.R:24: error: deleting env vars works
Forbidden (HTTP 403). Failed to delete env vars for repo 'pat-s/travis-testthat' on Circle CI.
Backtrace:
  1. testthat::expect_silent(...) tests/testthat/test-env-vars.R:24:2
  9. circle::delete_env_var(...)
 10. httr::stop_for_status(...) R/env-var.R:125:2
──────────────────────────────────────────────────────────────────────────────
x |   2 2     | general [0.7 s]
──────────────────────────────────────────────────────────────────────────────
test-general.R:21: error: triggering a new build works
Forbidden (HTTP 403). Failed to start a new build for repo pat-s/travis-testthat on Circle CI.
Backtrace:
 1. testthat::expect_s3_class(...) tests/testthat/test-general.R:21:6
 4. circle::new_build(repo = repo, user = user)
 5. httr::stop_for_status(...) R/general.R:114:2

test-general.R:31: error: checking the existence of checkout keys works
Forbidden (HTTP 403). Failed to get checkout keys for repo pat-s/travis-testthat on Circle CI.
Backtrace:
 1. testthat::expect_true(has_checkout_key()) tests/testthat/test-general.R:31:6
 4. circle::has_checkout_key()
 5. circle::get_checkout_keys(repo = repo, user = user, vcs_type = vcs_type) R/checkout-key.R:138:2
 6. httr::stop_for_status(...) R/checkout-key.R:67:2
──────────────────────────────────────────────────────────────────────────────
✓ |   4       | print S3 methods [8.7 s]

══ Results ═══════════════════════════════════════════════════════════════════
Duration: 21.0 s

OK:       9
Failed:   8
Warnings: 0
Skipped:  0

---

Reviewers:
Due date:

[pat-s]

Hi Noam,

thanks for taking this on!

My initial attempt to test this package failed

Tests are running on a test repo which is cloned during testing.
Since the repo lives in my account, other users won't have permissions to start/cancel builds.
We will also have to skip all tests on CRAN since there is no way I can set the API keys there.
The API keys are stored on the CI providers securely, so tests will run there for everyone.
We could potentially move the test repo to ropenscilabs though this would also just give a handful people access to it if they have local keys set up.
I'd say having the tests running on the CI for everyone is the best solution here.
Let me know if I overlook something.

Also for some tests I need a GITHUB_TOKEN with extended permissions (to delete a private GH user key of mine automatically which will be created by use_circle_deploy()).

I've added this piece of information to CONTRIBUTING.md as suggested and put a link into the README.

Also, to preempt another issue, your README is somewhat sparse and both it and the vignette lack an introduction to what the package is for and, importantly in this case, what it is not for.

I've added more information to the README.

In addition, I also optimized the Getting Started vignette. (You might want to wait a bit until the vignette is deployed because it seems there are some SSL certificate issues with pkgdown today).

Let me know if there are still points to address before the review can start.

[noamross]

Hi @pat-s, I don't think this is a great solution, because this means that a contributor or reviewer can't test the package, and if they do only pushing and having the PR run for you then you need to expose your extended-permission GITHUB_TOKEN (very insecure!) to all. I get that you will need to skip these on CRAN, but I'd much prefer that there be a full developer workflow. This should hopefully save you effort in the long run, as contributors will be able to test prior to submitting PRs. It's fine if this takes some setting up by the contributor.

A solution could be that the user provides an env var R_CIRCLE_TEST_REPO (forked from your repo if its contents are important), as well as an extended permissions github token.

[pat-s]

if they do only pushing and having the PR run for you then you need to expose your extended-permission GITHUB_TOKEN (very insecure!) to all

I don't see why anything should be exposed. The token is stored as a secure env var in the build and nothing will be exposed when a PR is run.

This should hopefully save you effort in the long run, as contributors will be able to test prior to submitting PRs.

I see the advantage of being able to run the tests locally.
However, maybe your are overlooking the following: Even if an API key is set, the reviewers would need my API key to make API calls to pat-s/travis-testthat to start/cancel build.

Even when setting their personal API key, reviewers would need to

• adjust the test suite manually so tests run on a repo of their own (possibly create that first)
• repo needs to have XY builds because tests are not just running on the most recent build/job
• repo needs to have a working Circle CI workflow

Because testing an API client package is not trivial, I created the dummy repo to have a dedicated repo for testing API functionality.
Sometimes things go wrong and nobody wants to have such things happening on a production repo.

If I would be a reviewer of such a package, I would certainly dislike having to do all of this effort just to run tests locally whereas they just run fine on the CI.

I'd argue that a API client package is special when it comes to testing, especially when it comes to CI client packages for which it is not sufficient to just replace the API key.

From my perspective, it is important that the functions of this package work if users apply them to a repo of themselves (after having set the API key) rather than requiring them to adjust the whole test suite.

I am not sure if there was a previous case like this one - maybe a general decision how such packages are handled by {ropensci} is needed here.
The next one ({travis}) is already waiting.

Happy to make adjustments if I get a pointer how to do so.
I just do not see one right now.

[noamross]

A very large number of our packages are actually API clients; circle's case is only a little different from several others. At a minimum we need a functioning test suite so that tests can pass locally and then later with rOpenSci's automatic checks. But there are a number of ways to accomplish this.

• Split tests into those that run with and without certain tokens available, and selectively run tests based on availability. Fewer tests may run locally but testthat::skip* functions make it transparent what is skipped. You'll see this pattern in arkdb. In this case it's an especially good idea to use mocking with the webmockr or httptest packages so that you can separate out tests on HTTP content from the actual response:
• Provide enough direction in CONTRIBUTING for someone to run the full test suite. e.g., get a token, fork/clone a repository, etc. This is the type of effort we expect from reviewers of packages, though a smaller number of contributors will likely go through it. Here's an example of more complex contributing directions written in response to a review. A few things would make this easier, for instance, using an environment variable (CIRCLE_TEST_REPO=http://...) to define the the URI of the test repo, avoiding the need to adjust the test suite.
• Minimize the testing effort by including set up / tear down the example repository as part of the test suite, thus only requiring the tester to provide keys with adequate scopes. I asked Jenny Bryan, who maintains a lot of packages with complex API interactions and auth setups (e.g. googledrive), about this case, and she strongly recommended a setup/teardown approach for long-term maintainability by both yourself and contributors. I won't require this full level of automation but it's an option in your toolbox.

In any case I'll leave it to reviewers to look at the completeness of the test suite.

Regarding this:

I don't see why anything should be exposed. The token is stored as a secure env var in the build and nothing will be exposed when a PR is run.

If you allow PRs to use your variables, all one needs to do is slip a Sys.getenv("GITHUB_TOKEN") and then a line of code like a curl to ship that value elsewhere in the PR. This is why CI systems pretty much all prevent such variables from being exposed to PR builds by default and it's highly recommended to leave it that way.

[pat-s]

Thank Noam, lots of good resources and pointers in there.
It might take me a while to come up with a test suite that fulfills all these requirements.

for long-term maintainability by both yourself and contributors.

I see the point even though it means a lot of work. Knowing this before might have scared me away even but now I'm in it ;)

In any case I'll leave it to reviewers to look at the completeness of the test suite.

Happy to wait for reviews until I implemented a more proper testing suite.

[noamross]

To clarify, @pat-s, we do need a test suite that passes locally in order to assign to reviewers, though they may have feedback on completeness or setup.

[noamross]

⚠️⚠️⚠️⚠️⚠️

In the interest of reducing load on reviewers and editors as we manage the COVID-19 crisis, rOpenSci is temporarily pausing new submissions for software peer review for 30 days (and possibly longer). Please check back here again after 17 April for updates.

In this period new submissions will not be handled, nor new reviewers assigned. Reviews and responses to reviews will be handled on a 'best effort' basis, but no follow-up reminders will be sent.

Other rOpenSci community activities continue. We express our continued great appreciation for the work of our authors and reviewers. Stay healthy and take care of one other.

The rOpenSci Editorial Board

⚠️⚠️⚠️⚠️⚠️

[annakrystalli]

⚠️⚠️⚠️⚠️⚠️
In the interest of reducing load on reviewers and editors as we manage the
COVID-19 crisis, rOpenSci new submissions for software peer review are paused.

In this period new submissions will not be handled, nor new reviewers assigned.
Reviews and responses to reviews will be handled on a 'best effort' basis, but
no follow-up reminders will be sent.
Other rOpenSci community activities continue.

Please check back here again after 25 May when we will be announcing plans to slowly start back up.

We express our continued great
appreciation for the work of our authors and reviewers. Stay healthy and take
care of one other.

The rOpenSci Editorial Board
⚠️⚠️⚠️⚠️⚠️

[noamross]

Hello @pat-s, we're starting up review activities so I wanted to check in with you on the status of this package. Last time we checked in you were developing a test suite in response to my editor's check. Any developments? I hope you're well and thank you for your patience!

[pat-s]

Hi Noam,

it's still on the list, the focus was on implementing an update mechanism in {tic} lately.

This is not an easy case here and I first have to review which mocking approach would be suited best for CI API calls. This would the also apply to {travis}.

I cannot give any ETA though.

[noamross]

No problem, thanks for the update!

[pat-s]

@noamross

Just wanted to inform you that I've started to incorporate {vcr} for http testing.
Still facing issues and lacking time but this project is not stalled (anymore) :)

[pat-s]

@noamross 👋
Happy to announce that the package is ready for review (again). Sorry that it took so long - a move between countries and COVID resulted in little time for FOSS packages, especially for niche ones.

I've completely revamped the test suite:

• now using {vcr}
• For local testing, env vars can be set (user, repo, api token) making it possible for reviewers to test against one of their repos
• Secrets like API keys are hidden in the http mocking approach of {vcr}

In addition, the following enhancements were made:

• Documentation now uses roxygen markdown
• testthat 3.0 ready
• pkgdown reference structure
• updated screenshots
• added parameter types to help pages
• added pre-commit hooks
• added codemeta

Also I've bumped the version to 0.6.0 (maybe this needs to be updated in the first post).

[noamross]

Thanks, @pat-s! I have run local tests and goodpractice::gp() checks and this passes with flying colors. I note that I had to run use_circle_deploy() once locally in addition to having my R_CIRCLE API key in the repository in order for local test flow to run. I'm now seeking reviewers.

[noamross]

Thanks @sharlagelfand and @mbjoseph for agreeing to review! Max is returning from leave, so the deadline will be extended a little longer until 2020-12-7.

[sharlagelfand]

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

• I have not had any working relationship with @pat-s.
• As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

• A statement of need clearly stating problems the software is designed to solve and its target audience in README
• Installation instructions: for the development version of package and any non-standard dependencies in README
• Vignette(s) demonstrating major functionality that runs successfully locally
• Function Documentation: for all exported functions in R help
• Examples for all exported functions in R Help that run successfully locally
• Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

• Installation: Installation succeeds as documented.
• Functionality: Any functional claims of the software been confirmed.
• Performance: Any performance claims of the software been confirmed.
• Automated tests: Unit tests cover essential functions of the package
  and a reasonable range of inputs and conditions. All tests pass on the local machine.
• Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Estimated hours spent reviewing: 4

• Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

---

Review Comments

I am very excited by the potential for this package! I have primarily used Travis and Github Actions in the past but have one project where I need to use Circle CI, and when I set it up wished there was a package to automate things the way that the {travis} and {ghactions} packages do. I believe that with improved documentation, this package will be very helpful for people wanting to automate their Circle CI setup and usage.

I have not checked off the majority of the "Documentation" section, with thoughts described in the sections below, but "Functionality" wise all looks good to me. Beyond what is mentioned below re: examples failing, I did not have trouble using the main functionality (getting jobs, starting builds, etc) and the test suite passed for me (I see above lots of discussion on getting it going - so thank you for that!).

Overall documentation

Much of the functionality in this package seems solid, but I think it is lacking in the documentation side of things, as Noam mentioned earlier. Neither the README nor the Get Started vignette provide much background into what Circle is or what Continuous Integration is for - you actually have great introductory documentation on this in ?circle-package so I would love to see that appear elsewhere! As Noam said, multiple points of entry are important, and I don't know if I would have found that page if I was not doing the package review.

On a related note, I would also love to see this page pop up when you do ?circle, rather than getting the documentation for the circle function - would you consider renaming the function and aliasing the documentation in circle-package to circle as well, since circle is explicitly never meant to be called by users?

It would be really great to see a few different vignettes for this package: one for existing users of Circle CI on how to automate their builds, etc, and one for newer users (or existing users who want to set up new repositories) on how to set up the entire thing, including how to use {circle} and {tic} in combination. The documentation states that the {tic} package should be used for setting up the YAML file, but does not give any more details or example of how this should be done. This content already has some start in the Quickstart section of the {tic} README but I believe it would be helpful to see it in both places. There is similarly great info on getting started in the Details section of use_circle_deploy() that could be brought more to the forefront.

I also think the Authentication section in "Get started" would benefit from more details (or even a screen shot, like in the Deployment section below) on how to get the API key. I found it difficult to struggle through the different authentication pieces, and a more detailed walk through would be great!

I found the functions very clear to use, with nicely written code and clear individual documentation! I really like how the arguments show what type they are (e.g. stating when an argument is a character versus a list) - this isn't something I've seen before.

Examples

Several examples from exported functions failed, listed below.

browse_circle_token

browse_github_token() errors below because it is part of the {usethis} package, which is not loaded in the example. Also, the examples section (shown below) only uses browse_github_token() and edit_circle_config(), but not browse_circle_token() itself.

library(circle)

browse_github_token()
# Error in browse_github_token() : 
#   could not find function "browse_github_token"

edit_circle_config()
# ● Modify '/Users/sharla/.circleci/cli.yml'

builds

Both get_workflows() and get_jobs() error because they are not exported, but are listed in the examples.

library(circle)

pipelines <- get_pipelines()

workflows <- get_workflows()
# Error in get_workflows() : could not find function "get_workflows"

jobs <- get_jobs()
# Error in get_jobs() : could not find function "get_jobs"

checkout_key

No fingerprint argument is given in the example, so the delete_checkout_key() function errors.

library(circle)

delete_checkout_key()
# Error in delete_checkout_key() : 
#  Please provide the fingerprint of the key which should be deleted.

get_build_artifacts

Errors, same as in the builds documentation, because get_jobs() is not exported.

library(circle)

job_id <- get_jobs()[[1]]$id
# Error in get_jobs() : could not find function "get_jobs"
get_build_artifacts(job_id)
# Error in get_build_artifacts(job_id) : object 'job_id' not found

Function usage

I love how chatty and responsive some of the functions are, like enable_repo() and use_circle_deploy()! I find it super helpful when functions tell you what they are doing and guide you to next steps. It would be great to see this extended to other functions like new_build(), which has important behaviour it could report back on, instead of returning API information, for example below (note I have redacted a couple IDs myself, not the function doing this!):

library(circle)

new_build()
# $content
# $content$number
# [1] 53
# 
# $content$state
# [1] "pending"
# 
# $content$id
# [1] [REDACTED]
# 
# $content$created_at
# [1] "2020-12-03T20:27:00.596Z"
# 
# 
# $path
# [1] "/project/gh/data-mermaid/mermaidr/pipeline"
# 
# $response
# Response [https://circleci.com/api/v2/project/gh/data-mermaid/mermaidr/pipeline?circle-token=[REDACTED]]
#   Date: 2020-12-03 20:27
#   Status: 201
#   Content-Type: application/json;charset=utf-8
#   Size: 115 B
# 
# 
# attr(,"class")
# [1] "circle_api"

I also noticed that get_pipelines() has a default repo = NULL, while (almost?) all of the other functions default to github_info()$name. It looks like the function actually converts the NULL to github_info()$name immediately, so may be worth updating for consistency.

I hope this is helpful - please let me know if you'd like me to clarify any points!

[mbjoseph]

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

• Briefly describe any working relationship you have (had) with the package authors.
• As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

• A statement of need clearly stating problems the software is designed to solve and its target audience in README
• Installation instructions: for the development version of package and any non-standard dependencies in README
• Vignette(s) demonstrating major functionality that runs successfully locally
• Function Documentation: for all exported functions in R help
• Examples for all exported functions in R Help that run successfully locally
• Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Functionality

• Installation: Installation succeeds as documented.
• Functionality: Any functional claims of the software been confirmed.
• Performance: Any performance claims of the software been confirmed.
• Automated tests: Unit tests cover essential functions of the package
  and a reasonable range of inputs and conditions. All tests pass on the local machine.
• Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Estimated hours spent reviewing:

• Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

---

Review Comments

Overall this package seems super useful for experienced Circle CI users, and there are a few small things that could be done to broaden the user base and make it easier for folks to get started. I'll echo the suggestions above about expanding the docs to include a bit more background on why this package is useful. Specific suggestions are provided below. At a high level, I like that the package provides good support for common operations with relatively high level abstractions, while also providing users access to the API via the general purpose circle::circle function. That's not an easy balance to strike, but I think this package does a good job. Finally, the code itself is clearly written and easy to follow - nice work!

Specific suggestions

• The documentation could present use cases for R users (maybe related to R package development, or websites built with R). This could be described in the README at a high level, and then in detail in a vignette. This would help would-be users understand what they get out of the package. Currently, with the way the README is written, it seems like users who don't know what the Circle CI REST API does might not understand whether they would benefit from using the package.

• The package documentation mentions deployments in a number of places. This might be a place where a novice gets tripped up. Is it worth demonstrating the functionality of the package for some deployment (maybe a website or shiny app) in a vignette?

• In this message it would be better to mention that the Project Settings > SSH Keys > ... stuff is on Circle CI's website, e.g., "On your repo's Circle CI project page, go to Project Settings > SSH Keys > ...".

• As Sharla mentioned some of the examples are failing because they call functions that aren't exported. It seems like part of the problem is that these examples are wrapped in dontrun, so they aren't raising errors upon checking, and another part of the problem is that some functions (e.g., get_jobs()) aren't exported. Both parts might point to different solutions, but either way it would be better for a user if the examples complete successfully.

• It would be helpful to document what functions return using @return, as this might not be obvious to users. For example, a user might want to get_build_artifacts, and the documentation can tell them what to expect as a return value when using that function.

[noamross]

Thanks @sharlagelfand and @mbjoseph for your reviews! @pat-s, please take a look and let us know what time frame you expect to be able to follow up with revisions.

[pat-s]

Thanks @sharlagelfand and @mbjoseph for your reviews. Since a lot of detail was provided, it will take me some time to reply and share my thoughts with you. Due to some redudandancy I'll submit a combined reply to both of your replies.

I cannot really give a realistic timeframe sicne the end of the year is usually quite busy in many regards - but I'll gradually come back here and work on my reply :)

[pat-s]

@sharlagelfand Thanks for your review!
Please note upfront that even though some of my comments might eventually sound a bit harsh, they are not meant to be like that but are just straight to the point :)

Neither the README nor the Get Started vignette provide much background into what Circle is or what Continuous Integration is for

The package is a client package - so I'd argue that users arriving here should know what they want to do with it. As a comparable example: a client package wrapping git (e.g. gert) also does not introduce git from scratch.
While I see the point of explaining function usage and returns in great detail, I do not think that this applies in the context of the upstream library/service/provider.
Links and cross-refs can help here but I do not want to re-explain CI.
I partly did this even in {tic} (re-explaining CI) but this package has arguably a much more wider scope than this client package here.

would you consider renaming the function and aliasing the documentation in circle-package to circle as well, since circle is explicitly never meant to be called by users?

Many packages name their workhorse function like the package name.
Also <package-name>-package is a common naming scheme for package "intro pages".
How about adding a top-level cross-ref to circle which states something like "for a package overview please have a look at XY"?
Given that this would only be a cosmetic change for users it would require quite some code reshuffling work.

It would be really great to see a few different vignettes for this package: one for existing users of Circle CI on how to automate their builds, etc, and one for newer users (or existing users who want to set up new repositories) on how to set up the entire thing, including how to use {circle} and {tic} in combination.

I agree that this would be benefitial.
However in practice the number of R users on Circle CI is probably less than 10 ;)
ropensci/circle#14

The documentation states that the {tic} package should be used for setting up the YAML file, but does not give any more details or example of how this should be done. This content already has some start in the Quickstart section of the {tic} README but I believe it would be helpful to see it in both places.

Yes, documentation for the {tic} integration could be improved.
However, {tic} is choice (and probably a biased one) so I want to be stay objective as possible and not bundle the packages too much.

There is similarly great info on getting started in the Details section of use_circle_deploy() that could be brought more to the forefront.

I will have a look how to bridge the gap between not repeating the help pages and not being to sparse in the vignettes.
I am not a friend of literally repeated text in help pages/vignettes but believe that vignettes should explain the bigger picture and help pages should provide more technical detail of a function.

I also think the Authentication section in "Get started" would benefit from more details (or even a screen shot, like in the Deployment section below) on how to get the API key.

While I see that this might be helpful in rare instances, I am not sure if I want to go down the "screenshot-explain-everthing" route. I do not see that much value in screenshot-explaining how to copy/create an API-key/Access token from a service.
Screenshots inherit maintenance work since they get outdate quickly if the UI gets updated, hence I try to avoid them unless they are really necessary.

I really like how the arguments show what type they are (e.g. stating when an argument is a character versus a list) - this isn't something I've seen before.

Thanks. Some packages have this but it always comes down to the maintainers. Definitive not a standard in R.

Several examples from exported functions failed, listed below.

Thanks, I'll have a look.
ropensci/circle#15

No fingerprint argument is given in the example, so the delete_checkout_key() function errors.

This is on purpose and that is why it is wrapped in dontrun. Most examples cannot run on CRAN due to API calls.
This particular function deletes an API key.
To delete an item, the item needs to be created first/exist already.

this extended to other functions like new_build(), which has important behaviour it could report back on,

Which important behaviour?

I also noticed that get_pipelines() has a default repo = NULL, while (almost?) all of the other functions default to github_info()$name. It looks like the function actually converts the NULL to github_info()$name immediately, so may be worth updating for consistency.

Thanks, might be an oversight.
ropensci/circle#16

[sharlagelfand]

Hey @pat-s, thanks for your response and for creating the linked issues!

I don't think that you necessarily need to explain what Continuous Integration is, but it would be helpful for the entry README and Get Started vignette to at least contain the phrase "Continuous Integration" - I like level of detail in {usethis} use_travis and use_actions, personally.

Re: vignettes and re-explaining things, I'll refer to the rOpenSci Packages book's section on documentation, namely (paraphrasing is mine):

• Packages should contain a vignette illustrating realistic use cases
• If your package connects to an online service, it should provide enough information for users to understand the nature of the service - any vignette should outline prerequisite knowledge to understand the vignette upfront.
• README, top-level docs, vignettes, etc should have enough information to get a high-level overview and to follow the principle of multiple points of entry (which Noam referenced)

Yes, documentation for the {tic} integration could be improved.
However, {tic} is choice (and probably a biased one) so I want to be stay objective as possible and not bundle the packages too much.

Re: integration with {tic} - that's fine with me if you don't want to bundle them. Would you be able to, instead, include or link an example of a YAML template so that users can get set up?

This is on purpose and that is why it is wrapped in dontrun. Most examples cannot run on CRAN due to API calls.
This particular function deletes an API key.
To delete an item, the item needs to be created first/exist already.

My understanding of the example section is that it contains things a user would actually be able to run - it should illustrate actual usage and not only include a function that you expect to fail. Being wrapped in dontrun because it will fail on CRAN is one thing - failing locally is another.

If an item needs to first be created, could the example instead be:

• Create the item (and store the ID in an object)
• Delete the item

Would that work? Related, I realized I don't even know what a "checkout key" is in Circle CI - the only thing that comes up on google is the {circle} package itself. Could you link to the relevant documentation?

Which important behaviour?

Sorry, was too vague here - important behaviour meaning it triggers a build. As a user, I would be more receptive to and better understand a function that returns e.g. "✓ New build started" rather than API call information.

I imagine that you and other folks will also be taking some time off for the holidays - I will be mostly offline until December 29, so will respond to anything new after that 😊 Happy holidays!

[pat-s]

@mbjoseph Thanks for taking the time to review!

Overall this package seems super useful for experienced Circle CI users, and there are a few small things that could be done to broaden the user base and make it easier for folks to get started. I'll echo the suggestions above about expanding the docs to include a bit more background on why this package is useful.

Thanks. I'll echo my thoughts from the previous reply to Sharla that I am not movitated to re-explain CI (even though you did not mention this explicitly) - see my reasonings in the previous reply.
As for a general update to the "Getting Started" docs I note your vote and will try to simplify the onboarding experience.

At a high level, I like that the package provides good support for common operations with relatively high level abstractions, while also providing users access to the API via the general purpose circle::circle function.

Thanks.
Indeed this is complex on a meta level and I am not sure if I got it correct.
I might to make it more clear that circle() represents the direct way and that many high-level functions exists but one can still make plain API calls.

The documentation could present use cases for R users (maybe related to R package development, or websites built with R). This could be described in the README at a high level, and then in detail in a vignette. This would help would-be users understand what they get out of the package. Currently, with the way the README is written, it seems like users who don't know what the Circle CI REST API does might not understand whether they would benefit from using the package.

From my view {circle} serves the primary use case of interacting with the API while also providing some sugar functions to make tedious task easier (such as adding keys).
Given that I am also authoring {tic} which aims to simplify CI at a more general level and uses {circle} as a client package, I believe that workflow abstraction examples should rather live there.

The sugar functions and everything else should be descriptive by their names so that users can easily get some data if they want to.
On the other hand, there are too many endpoints in an API to know what users possibly might want to do with them.

In this message it would be better to mention that the Project Settings > SSH Keys > ... stuff is on Circle CI's website, e.g., "On your repo's Circle CI project page, go to Project Settings > SSH Keys > ...".

Thanks, noted!

As Sharla mentioned some of the examples are failing because they call functions that aren't exported. It seems like part of the problem is that these examples are wrapped in dontrun, so they aren't raising errors upon checking, and another part of the problem is that some functions (e.g., get_jobs()) aren't exported. Both parts might point to different solutions, but either way it would be better for a user if the examples complete successfully.

Thanks, already fixed in ropensci/circle#17.

It would be helpful to document what functions return using @return, as this might not be obvious to users. For example, a user might want to get_build_artifacts, and the documentation can tell them what to expect as a return value when using that function.

Thanks, noted!

[mbjoseph]

Hey @pat-s - apologies for the delay here, but thanks for responding to my review! Changes look good to me, and where you haven't made suggested changes I can see your perspective (especially in light of what tic does). 👍 Nice work.

[sharlagelfand]

Apologies for the delay on my end as well @pat-s! Agreed, changes look good to me. I really like the intro material in the README, and the {tic} vignette - especially walking through the YAML file and explaining what each section is doing, what a good idea :)

All looks good to me, will update my checklist ^^^ way above.

[pat-s]

Thanks @mbjoseph and @sharlagelfand!

I still got some replies in draft mode - just did not find the time yet to finish them. I'll try to wrap them up in the next days.

Apologies for the delay also on my side, this project is apparently not prio no.1 right now :)

[pat-s]

@sharlagelfand

Re: integration with {tic} - that's fine with me if you don't want to bundle them. Would you be able to, instead, include or link an example of a YAML template so that users can get set up?

Thanks. As you've already seen there is a new vignette about tic & circle.

My understanding of the example section is that it contains things a user would actually be able to run - it should illustrate actual usage and not only include a function that you expect to fail. Being wrapped in dontrun because it will fail on CRAN is one thing - failing locally is another.
If an item needs to first be created, could the example instead be:
Create the item (and store the ID in an object)
Delete the item

I see your point here and were arguably a bit lazy with including meaningful examples.

Would that work? Related, I realized I don't even know what a "checkout key" is in Circle CI - the only thing that comes up on google is the {circle} package itself. Could you link to the relevant documentation?

Interesting. I'll add some more info to the function help page.

Sorry, was too vague here - important behaviour meaning it triggers a build. As a user, I would be more receptive to and better understand a function that returns e.g. "✓ New build started" rather than API call information.

Ok thanks, I see.
I think the API information is important and contains metadata which users might want to use for certain postprocessing steps.
I think we could combine both - returning the API info as now and add some verbose {cli} outputs.

[pat-s]

I've wrapped up all review-related changes in v0.7.0.

Thanks to @sharlagelfand and @mbjoseph again 👏

@noamross Looks like we can proceed here.

[pat-s]

Hi @noamross

Just checking base for what might be the next steps from your side - the plan is to release to CRAN after this review is finished.

[noamross]

Sorry I let this fall through the cracks - I was out for the period where these last parts wrapped up.

[noamross]

@ropensci-review-bot approve

[ropensci-review-bot]

Approved! Thanks @pat-s for submitting and @sharlagelfand, @mbjoseph for your reviews! 😁

To-dos:

• Transfer the repo to rOpenSci's "ropensci" GitHub organization under "Settings" in your repo. I have invited you to a team that should allow you to do so. You'll be made admin once you do.
• Fix all links to the GitHub repo to point to the repo under the ropensci organization.
• Delete your current code of conduct file if you had one since rOpenSci's default one will apply, see https://devguide.ropensci.org/collaboration.html#coc-file
• If you already had a pkgdown website and are ok relying only on rOpenSci central docs building and branding,
  • deactivate the automatic deployment you might have set up
  • remove styling tweaks from your pkgdown config but keep that config file
  • replace the whole current pkgdown website with a redirecting page
  • replace your package docs URL with https://docs.ropensci.org/package_name
  • In addition, in your DESCRIPTION file, include the docs link in the URL field alongside the link to the GitHub repository, e.g.: URL: https://docs.ropensci.org/foobar (website) https://github.com/ropensci/foobar
• Fix any links in badges for CI and coverage to point to the ropensci URL. We no longer transfer Appveyor projects to ropensci Appveyor account so after transfer of your repo to rOpenSci's "ropensci" GitHub organization the badge should be [![AppVeyor Build Status](https://ci.appveyor.com/api/projects/status/github/ropensci/pkgname?branch=master&svg=true)](https://ci.appveyor.com/project/individualaccount/pkgname). If Appveyor does not pick up new commits after transfer, you might need to delete and re-create the Appveyor project. (Repo transfers are smoother with GitHub Actions)
• Please check you updated the package version post-review version updated and that you documented all changes in NEWS.md
• We're starting to roll out software metadata files to all ropensci packages via the Codemeta initiative, see https://github.com/ropensci/codemetar/#codemetar for how to include it in your package, after installing the package - should be easy as running codemetar::write_codemeta() in the root of your package.

Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent). More info on this here.

Welcome aboard! We'd love to host a post about your package - either a short introduction to it with an example for a technical audience or a longer post with some narrative about its development or something you learned, and an example of its use for a broader readership. If you are interested, consult the blog guide, and tag @stefaniebutland in your reply. She will get in touch about timing and can answer any questions.

We've put together an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here.

[pat-s]

@sharlagelfand @mbjoseph Are you ok being acknowledged as a reviewer in the package? Please thumbs up if so.

Thanks again for reviewing! 🙇