Submission: tic #305
Comments
|
Hello @krlmlr! 👋 I'll be your editor on this submission. Will be back in touch when I've completed the editor checks. 👍 |
Editor checks:
Editor commentsHello again @krlmlr, I've completed the editors checks, see below for issues raised. Feel free to address/respond to any issues raised. Can I also please ask that you add the rOpenSci review badge to the README? In the meantime, I'll start looking for reviewers! checksThe checks throw up an example which isn't working: #> ── R CMD check results ─────────────────────────────── tic 0.2.13.9016 ────
#> Duration: 54.8s
#>
#> ❯ checking examples ... ERROR
#> Running examples in ‘tic-Ex.R’ failed
#> The error most likely occurred in:
#>
#> > base::assign(".ptime", proc.time(), pos = "CheckExEnv")
#> > ### Name: step_do_push_deploy
#> > ### Title: Step: Perform push deploy
#> > ### Aliases: step_do_push_deploy
#> >
#> > ### ** Examples
#> >
#> > dsl_init()
#> �[32m✔�[39m Creating a blank tic stage configuration
#> >
#> > get_stage("deploy") %>%
#> + add_step(step_do_push_deploy(path = "docs"))
#> Error: Error evaluating the step argument of add_step(), expected an object of class TicStep.
#> Original error: The working directory is not in a git repository
#> Execution halted
#>
#> 1 error ✖ | 0 warnings ✔ | 0 notes ✔
#> Error: R CMD check found ERRORsCreated on 2019-05-29 by the reprex package (v0.3.0) tests
pkg_dir <- "/Users/Anna/Documents/workflows/rOpenSci/editorials/tic"
devtools::test(pkg_dir)
⠙ | 1 1 | test-integration-git.R
✔ | 1 1 | test-integration-git.R [0.8 s]
#> ───────────────────────────────────────────────────────────────────────────────────
#> test-integration-git.R:19: warning: integration test: git
#> `recursive` is deprecated, please use `recurse` instead
#> ───────────────────────────────────────────────────────────────────────────────────
#>
⠏ | 0 | test-integration-package.R┌───────────────────────────────────────┐
#> │ │
#> │ integration test: package failure │
#> │ │
#> └───────────────────────────────────────┘
#>
#> Error : A step failed in stage "script": script.
#>
⠹ | 1 2 | test-integration-package.R
✔ | 1 2 | test-integration-package.R [6.2 s]
#> ───────────────────────────────────────────────────────────────────────────────────
#> test-integration-package-failure.R:9: warning: integration test: package failure
#> `recursive` is deprecated, please use `recurse` instead
#>
#> test-integration-package-failure.R:9: warning: integration test: package failure
#> `recursive` is deprecated, please use `recurse` instead
#> ───────────────────────────────────────────────────────────────────────────────────
#>
⠏ | 0 | test-integration-package.R┌───────────────────────────────┐
#> │ │
#> │ integration test: package │
#> │ │
#> └───────────────────────────────┘
⠹ | 1 2 | test-integration-package.R
✔ | 1 2 | test-integration-package.R [5.6 s]
#> ───────────────────────────────────────────────────────────────────────────────────
#> test-integration-package.R:9: warning: integration test: package
#> `recursive` is deprecated, please use `recurse` instead
#>
#> test-integration-package.R:9: warning: integration test: package
#> `recursive` is deprecated, please use `recurse` instead
#> ───────────────────────────────────────────────────────────────────────────────────
#>
⠏ | 0 | test-repo
⠋ | 1 | test-repo
⠙ | 2 | test-repo
⠹ | 3 | test-repo
⠸ | 4 | test-repo
✔ | 4 | test-repo [3.9 s]
#>
⠏ | 0 | test-stages.R
✔ | 2 | test-stages.R
#>
⠏ | 0 | tasks-base
✔ | 6 | tasks-base
#>
⠏ | 0 | test-utils.R
✔ | 4 | test-utils.R
#>
#> ══ Results ════════════════════════════════════════════════════════════════════════
#> Duration: 24.3 s
#>
#> OK: 64
#> Failed: 0
#> Warnings: 5
#> Skipped: 0Created on 2019-05-29 by the reprex package (v0.3.0) goodpracticeOutput from spellingA few candidates for genuine typos identified through Finally, picking up from the coverage issue picked up by In general, we require atleast 75% test coverage. Is there a reason coverage is at ~50% and that a number of Comments for reviewers
|
|
We know have reviewers! 🎉 Many thanks @ldecicco-USGS & @maxheld83 for agreeing to review. Any questions, please feel free to reach out. Reviewers: @ldecicco-USGS, @maxheld83 |
|
Thanks. I've addressed the editorial comments in v0.2.13.9017:
The comments regarding coverage helped identify a smell in the API, thanks! Working on improving it, will report here. {rlang} and {cli} are sufficiently safe to be imported as a whole, {backports} really needs to be imported as a whole. |
|
That API smell is removed in v0.2.13.9018. Looking forward to your reviews! |
|
Can someone remind me where to find the blank reviewer templates? |
|
Hey Laura,
You can find the template here: https://devguide.ropensci.org/reviewtemplate.html
…Sent from my iPhone
On 19 Jun 2019, at 18:08, Laura DeCicco ***@***.***> wrote:
Can someone remind me where to find the blank reviewer templates?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
As discussed with editor @annakrystalli, I am the maintainer of the ghactions package for R workflow automation with GitHub Actions. DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 3
Review CommentsCI/CD is extremely important for open and reproducible research, and the tic package is an incredibly ambitious, generous and comprehensive initiative to make it easier for more people to adhere to these best practices. The promotion of CI/CD for non-package projects is particular helpful for research purposes, and somewhat unaddressed so far. The package is thoroughly documented, with great introductory vignettes for newcomers. So: definetely accept this great contribution! I do have some reservations towards the agnostic philosophy of tic, though this isn't something I would bet on, let alone to reject this. Perhaps my thoughts can still be valuable for the authors. I'm a bit skeptical as to whether it's a good idea to wrap yet another layer on top of the (usually) There is an obvious appeal to abstracting over different providers, and for making this so smooth, but there are also some potential downsides:
I've learned CI/CD "the hard way", with many wasted hours editing But I wonder whether the tradeoff is real. |
|
Thank you for your very thoughtful review @maxheld83 ! Stay tuned for @krlmlr's response once all reviews are in. |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Note: Many help pages didn't have examples. I find examples really useful. Most of the functions that did have examples started with
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 4
Review CommentsFull disclosure, I am not a I couldn't figure out a way to use the package just by browsing the help files. That's not too bad...not all packages will work that way. It just happens to be what I do when I install a new package. So..., I started with the "Get Started" link: The first thing I did was run The thing that's frustrated me with CI stuff lately is that I have a great handle on Travis, but for some reason appveyor has been giving me all sorts of headaches. So, I like that theoretically I don't need to figure out appveyor. That being said, it's been empowering to me to understand the ins-and-outs of Travis. My FIRST impression: My tough question is....is this package a better use of development/maintenance resources compared to maintaining educational resources (blogs, tutorials, book?) on how to do it directly? Basically, weigh the risk/reward of having people "addicted" to this package vs creating their yml's from scratch. Reward: when R-users can't figure out their CI...they have the package maintainers of Also as an aside, my organization is strongly encouraging us to use a gitlab system, so now I need to figure out how to set up a ".gitlab-ci.yml" file. A blog like this: My SECOND impression: There are some great examples and use cases within the vignettes. I wonder if the "Get Started" page could be re-written to highlight those right away, to get users a reason to buy-in to the package concept (before running the So for example...adding something right up front like: You might already know that travis can:
But did you know it could also:
If each of those bullets had a link to a place that described that concept in detail, coool. My last impression (during the review): I do think this is a useful package. If it gets published and well publicize, I think there could be many users that depend on it (perhaps heavily). As long as it's properly maintained, that's great. If there isn't long-term support for the package... as a user, I'd consider that a high-risk to me, and might not bother with it. Overall, 💯 , nice package, people will like it, and a few tweeks to the documentation might get MORE people to like it....but it's up to the developers to decide if that's a good thing. |
|
Thank you @ldecicco-USGS for your insightful review! Over to you @krlmlr. I hope the reviews have provided some food for thought. They certainly have for me! |
annakrystalli
added
4/review(s)-in-awaiting-changes
and removed
3/reviewer(s)-assigned
labels
|
Hi @krlmlr, just touching base on this review, although I appreciate it is vacation season. Just let me know if for whatever reason you need more time to respond. |
|
Hi all, since @krlmlr seems to be very busy right now, I'll take on replying to the reviewers comments. Review from @ldecicco-USGS (Laura DeCicco)
All steps that are being called by
The ultimate goal is to be CI-agnostic. Specify what you want to do using R code as if you were working locally. Do not worry about the CI yml files anymore (in 90% of the time). tic helps you also in writing R code by providing macros that do common things (install deps, run R cmd check, build and deploy pkgdown). You are always free to just write these steps in R code yourself.
I think its hard to say what the best way is to promote the package/start with. Some people will always read first while others will always just execute code right away and "see what happens". It also depends on the level of experience with CI systems. Exp users will have a very different take on incorporating tic than unexperienced ones.
This looks like a good structure that we indeed could implement in the README to get people on board.
Thanks. Maintenance in this case means mainly the setup functions which mainly live in the travis pkg. Everything else (macros, etc). is basically sugar for the user and input from the community is highly welcome. Thanks for your review! I'll take the following actions items out of it:
|
Review from @maxheld83
Yes, they are already a layer on top of the CI system. However, these just install a basic toolchain for the respective language. Then again there needs to be a community-driven effort of maintaining that structure which is in fact open-source but somewhat hidden and hard to understand for average people. The second big advantage is the simplification of complex tasks.
True, this will be the case or at least a potential downside that thism pkg comes with.
This is also a valid concern. There is no way around having manual checks/updates for the compliancy of the template. However, we have example repositories running with the CI template files which should detect upstream changes early.
I would argue that this point is very subjective - a lot of users have huge problems understanding CI ymls. The ones who are not just deal with it frequently.
The user is not bothered with R6 as all exported functionality are simple functions and not R6 classes to interact with.
Correct. However, this project probably needs a lot more man power and serious funding behind it. And then again, R would probably not the first language to go for in that case. So I think there is no way but helping yourself here (yourself = R community).
Definitely, I have not checked on this yet.
I agree but then again it is not our task to decide on this. This pkg is not just aiming at helping the "non-programming" R user but also for advanced people to simplify their workflow across providers.
On the other hand: What if Travis shuts its doors and the R com needs to move to another provider in the future? Then all the "Travis knowledge" is worth nothing. However, knowing a CI-agnostic system is key then.
This is a good point to take-away. Thanks @maxheld83 for your detailed thoughts on this! I'll share most of your concerns. However, most of them are also on a meta-level and hard to deal with. We will not know how the system develops and what might be the best approach to tackle CI workflows in the future. tic offers one way (a CI-agnostic R DSL) and users need to decide whether they want to hop on that train or not. Things I take away:
|
|
I know it's been quite some time now but: Are there still open points for discussion from your review? Reading the comments history, I see that @krlmlr has already addressed your review in #305 (comment). As most of the comments from the other two reviews were on the meta site and somewhat hard to address/subjective, I "only" have the three action items listed at the bottom of my replies the reviews. Do you still see any blocking issues apart from these points I listed that would block the transitioning to ropensci? Again a big thanks to all reviewers (@annakrystalli, @ldecicco-USGS, @maxheld83 ) from my side and apologies for the late responses here. |
|
Hello @pat-s ! Great to hear from you, thanks for picking this up and for the detailed comments. I'll allow @ldecicco-USGS & @maxheld83 to respond to them first. If they are both happy then the package will be accepted when the last points have been address. In general, it all sounds sensible to me but I'll defer to the reviewers for now. |
annakrystalli
added
5/awaiting-reviewer(s)-response
and removed
4/review(s)-in-awaiting-changes
labels
|
I'm happy! Thanks so much @pat-s for the thorough responses to my review, which I hope was helpful for the authors as well. Amazing work. |
|
Sounds great, I fully 👍 this package. Look for a feature request Github issue as I'm currently knee-deep in converting all my Travis jobs to .gitlab-ci.yml.... 😿 |
|
Excellent! Thank you both! And sorry to hear that enforced migration is upon you @ldecicco-USGS. So, as far as I'm concerned, please go ahead and make the last few changes you proposed @pat-s. When you're done, ping me here and I'll walk you through the last approval stages. Thanks everyone! 👏 |
|
Hi everyone, @krlmlr and I decided to make the workhorse function Also the user can soon choose between three platforms (circle ci, travis ci, appveyor) and make choices whether to
Depending on the choices different actions will be taken (e.g. setting up deployment keys for specific providers) and specific YAML templates will be selected. We are now also using the cli package for the interactive messages. This, in combination with ongoing work in the travis package (with a full refactoring of the auth process and no more browser tab popups) and the new circle package (for interfacing the Circle CI API) should give the whole project a new kickstart and make it a lot easier for users. We will meet in person at the end of Nov to finalize things. Until then, we hope that ropensci/tic#193 is finished and all the other changes are done so that we finally can go to ropensci and CRAN. We would appreciate user feedback for ropensci/tic#193 if you have some minutes to spare 🙏 |
|
Sounds like good progress @pat-s ! Just to confirm, the package is likely to be finalised (and therefore all the issues identified in the review will be addressed) by the end of November? |
|
tic is now ready for migration to ropensci from our side. The CRAN roadmap is as follows:
Regarding tic specifically:
We've done a major refactoring to how We believe to have addressed all reviewer points and would like to thank again all people who have contributed to this. Sorry that everything took a bit long (from our side) and thanks for your understanding :) |
|
Hi @pat-s, Thanks for the updates on the package! I'm excited to test it out but I'm getting an error: tic::use_tic()
#> Error in cli_alert("Welcome to {.pkg tic}!"): could not find function "cli_alert"Created on 2019-12-10 by the reprex package (v0.3.0). Indeed running These functions are from package |
|
@annakrystalli cli v2.0.0 is on CRAN since yesterday and installing tic should now get you the correct version during install. |
|
Great thanks. I'll take a look again tonight 👍 |
|
Hey @pat-s, Package installed successfully! 🎉 However, now at the end of The page it launches though seems to show travis is sufficiently authorised? Any idea why authorization is failing? It seemed to be working before. |
|
Hm, tricky. This check comes from It is now easy to say what might be the problem here. You could
Other than that, I cannot reproduce the issue on my machine and we might dig deeper in a screenshare or similar if the issue persists. However, the issue you encountered is in the end caused by the {travis} pkg and not by {tic}. Nevertheless I understand if you consider this as blocking because both packages are coupled closely, even though it is strictly triggered by an imported package. Permission stuff with 3rd party providers is tricky as one cannot/hardly create automated tests for this. This is when user feedback becomes very important to sort things out, so thanks a lot! |
|
Hello again @pat-s 👋 OK, after installing the branch you suggested, things worked better. I still had a bit of trouble figuring out the setup. Finally, once I figured that the However, sadly the resulting build fails The pertinent error being: I reproduced this on another repo too Additional commentsLike you say users need GITHUB authentication set up and travis (and any other service required) installed and authorised on GitHub (which can be even more complicated with orgs). As it is hard to automate troubleshooting all this, the docs will have to do the job. While error messages are generally informative, some are more cryptic than others so the travisin the docs with a clear description of the various set up stages for each CI and anticipating searches for relevant information would be useful. Perhaps expanding on the Ultimately, users shouldn't have to go to docs in other packages for basic set up instructions for using Here's some more specificing suggestions for improvements to documentation and functionality:
|
|
Hi @annakrystalli, Thanks for the detailed review again. Important points, especially when it comes to user experience. Do you consider all of these points (including some that only target the {travis} package) as blocking for the migration to ropensci? Maybe we can put them into issues of the respective repos to better address them? (Migrating to ropensci won't stop development therefore, especially because the package is not on CRAN yet. I am wondering if/how we can proceed with this submission here in the most effective way). |
Thanks for catching this. Replacing |
|
OK thanks. I've updated I'm just using tic vanilla bookdown setup https://github.com/annakrystalli/reprohack-docs. Is a DESCRIPTION file necessary for this to work? |
Yes, because {{tic}} installs the dependencies based on that. No matter the project type, there always needs to be a DESCRIPTION file. |
|
SUCCESS!! 🎉 So, If the project requires it, I feel If I try and use Indeed, if Regarding your previous questions, I don't feel review can be complete without some of the documentation issues being addressed. It took a lot of time to try and get what should have been a simple test project up and running so I imagine that many people might find setup confusing as it stands. I also can't accept |
I see. We can check for this during I've created an issue to better assert the existence of a DESCRIPTION file.
The refactoring should be done within the next days/weeks. In general, {tic} should also work with the current state of the {travis} package, even though it is limited to the .org endpoint only and its usage is a bit more annoying than the refactored version in the Could we quickly summarize which issues are blocking for the transition? I'd like to outsource your points from #305 (comment) into issues in the repo and label them accordingly. You might as well do it on your own if you want to add more context to certain points :) |
|
I think you should incorporate all the comments as issues as you feel they make sense. I've ticked the ones I would consider necessary to pass review, the rest you can add as enhancements. But it would be good to get a full response to the points raised once you have made changes.
The problem with that is that since May 2018, travis has announced a movement towards |
There is only one app left that can be actively installed. I written some sentences about this in ropensci/tic#213
Done in ropensci/tic#213
Done in ropensci-archive/travis@a8b7ad3
I don't think that this belong into the {tic} package but rather into the {travis} package. The {tic} package has already so many vignettes and we need to take care not to bloat up the package too much. We can't write a vignette about the specialties of all existing and upcoming backends. There is already a section about how to set
I see your concern here but disagree. We would like not to make an opinionated statement which endpoint to use and if one should fully migrate or not. This is a decision the user needs to make. Regarding passing the
I am not sure about such a vignette. Most people prefer searching error messages and not many people will read a vignette upfront before trying out the functions. Also I would not know right now what to put there.
There is a small example in https://docs.ropensci.org/tic/articles/tic.html already. I am not sure what you mean by "not breaking down the questions"? We hope that
I think users interested in a package simplifying CI usage are familiar enough with git/Github to know that their local repo needs to be linked. The error message is even quite descriptive imo. I don't think that checking this is within the responsibility of this package.
{tic} macro-functions and steps (i.e. all funs starting with "do*" or "step" are not aimed to be run standalone. We now return early if those functions are used interactively. Also we now suggest to call
The {travis} refactoring is done.
We will stick with https://docs.ropensci.org/tic until we are on CRAN. |
|
library(tic)
dsl_init()
#> ✓ Creating a clean tic stage configuration
#> ℹ See `?tic::dsl_get` for details
do_bookdown()
#> Using TRAVIS_DEPLOY_KEY env var as the private key name for SSH deployment.
#>
#> ── tic configuration ────────────────────────────────────────────────────────────────────────────────────
#> ── install ───────────────────────────────────────────────────────────────────────────────────── stage ──
#> ▶ step_install_deps(repos = repo_default())
#> ── deploy ────────────────────────────────────────────────────────────────────────────────────── stage ──
#> ▶ step_build_bookdown()Created on 2020-01-13 by the reprex package (v0.3.0) I wonder if the link to the help page should be printed every time a configuration is shown, or just once (as in the example above). |
|
Thanks for all your work! I feel all my comments have been addressed apart from the following:
Because this environmental variable needs to be set for The rest are just some responses to some of your comments @pat-s and general suggestions but no action is necessary to pass review.
As a user that tried to set up
Don't worry about this. I am likely being pedantic in that I felt it would make more sense to be explicit about the code being in a
That's fine. The error message is ok. In general though I recommend you do not assume too much knowledge from your users, especially in technical terminology, (eg remotes) and try and help them debug as much as possible (which is why I made the suggestion). It will make your packages much nicer to work with for beginners. For @krlmlr comment:
That's great.
All in all we are pretty much there. If you could just address the one comment that still requires action we'll be ready for approval! 😃 |
|
Thanks. I've added links to the relevant documentation for setup of environment variables to the |
|
Approved! 🥳 Thanks @krlmlr & @pat-s for submitting and @ldecicco-USGS & @maxheld83 for your reviews! 😸 To-dos:
Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them Welcome aboard! We'd love to host a blog post about your package - either a short introduction to it with one example or a longer post with some narrative about its development or something you learned, and an example of its use. If you are interested, review the instructions, 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. |
|
@annakrystalli Thanks a lot for your patience and your awesome review. I addressed all the points and just transferred the repo and assigned it to team "tic". |
|
Great, thanks @pat-s ! It looks like you and @krlmlr still have full admin rights so I think we're all done here and I can close the issue. I also think it would be great to announce the package to the rOpenSci community so do consider writing a blogpost/technical note if you have time/the inclination |
Submitting Author: Kirill Müller (@krlmlr)
Repository: ropenscilabs/tic
Version submitted: 0.2.13.0916
Editor: @annakrystalli
Reviewer 1: @ldecicco-USGS
Reviewer 2: @maxheld83
Archive: TBD
Version accepted: 0.3.0.9004
DESCRIPTIONfileScope
Please indicate which category or categories from our package fit policies this package falls under:
Explain how and why the package falls under these categories (briefly, 1-2 sentences):
Technical checks
Confirm each of the following by checking the box. This package:
Publication options
JOSS Options
paper.mdmatching JOSS's requirements with a high-level description in the package root or ininst/.MEE Options
Code of conduct
CC @pat-s @maelle @karthik
The text was updated successfully, but these errors were encountered: