search

ropensci / software-review

rOpenSci Software Peer Review.
295 stars 104 forks source link

Submission: circle #356

Closed pat-s closed 3 years ago

pat-s commented 4 years ago

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

Due date for @sharlagelfand: 2020-12-07 Due date for @mbjoseph: 2020-12-07

Archive: TBD
Version accepted: TBD 

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

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

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

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

Technical checks

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

Publication options

JOSS Options - [ ] The package has an **obvious research application** according to [JOSS's definition](https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements). - [ ] The package contains a `paper.md` matching [JOSS's requirements](https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain) 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*)
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](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/journal-resources/policy-on-publishing-code.html)) - (*Scope: Do consider MEE's [Aims and Scope](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/aims-and-scope/read-full-aims-and-scope.html) 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

maelle commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

noamross commented 4 years ago

Editor checks:

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

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 commented 4 years ago

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.

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

⚠️⚠️⚠️⚠️⚠️

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 commented 4 years ago

⚠️⚠️⚠️⚠️⚠️ 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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

No problem, thanks for the update!

pat-s commented 4 years ago

@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 commented 4 years ago

@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:

In addition, the following enhancements were made:

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

noamross commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

Documentation

The package includes all the following forms of documentation:

Functionality

Estimated hours spent reviewing: 4

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 commented 3 years ago

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

Documentation

The package includes all the following forms of documentation:

Functionality

Estimated hours spent reviewing:

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

noamross commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 ;) https://github.com/ropenscilabs/circle/issues/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. https://github.com/ropenscilabs/circle/issues/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. https://github.com/ropenscilabs/circle/issues/16

sharlagelfand commented 3 years ago

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):

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:

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 commented 3 years ago

@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 https://github.com/ropenscilabs/circle/pull/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 commented 3 years ago

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). :+1: Nice work.

sharlagelfand commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

noamross commented 3 years ago

@ropensci-review-bot approve

ropensci-review-bot commented 3 years ago

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

To-dos:

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 commented 3 years ago

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

Thanks again for reviewing! 🙇