Closed krlmlr closed 4 years ago
Hello @krlmlr! π
I'll be your editor on this submission. Will be back in touch when I've completed the editor checks. π
Hello 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?
[![](https://badges.ropensci.org/305_status.svg)](https://github.com/ropensci/software-review/issues/305)
In the meantime, I'll start looking for reviewers!
The 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 ERRORs
Created on 2019-05-29 by the reprex package (v0.3.0)
devtools:test()
is throwing up some warnings for me (well the same warning in a couple of locations):
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: 0
Created on 2019-05-29 by the reprex package (v0.3.0)
Output from goodpractice::gp(pkg_dir)
It is good practice to
β write unit tests for all functions, and all package code in general. 51% of code lines are covered by test
cases.
R/base64.R:15:NA
R/base64.R:16:NA
R/base64.R:17:NA
R/base64.R:18:NA
R/base64.R:31:NA
... and 368 more lines
β avoid long code lines, it is bad for readability. Also, many people prefer editor windows that are about 80
characters wide. Try make your lines shorter than 80 characters
R/appveyor.R:24:1
R/ci.R:45:1
R/dsl-storage.R:81:1
R/dsl-storage.R:85:1
R/dsl-storage.R:110:1
... and 88 more lines
β not import packages as a whole, as this can cause name clashes between the imported packages. Instead, import
only the specific functions you need.
βββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββββ
A few candidates for genuine typos identified through devtools::spell_check()
DESCRIPTION does not contain 'Language' field. Defaulting to 'en-US'.
WORD FOUND IN
beim tic.Rmd:221
builded NEWS.md:195
commited deployment.Rmd:166
constitutent dsl.Rd:37
interfer tic.Rmd:129
undefinied NEWS.md:227
ususally build-lifecycle.Rmd:20
Finally, picking up from the coverage issue picked up by gp()
, coverage()
breaks down the problem areas in a bit more detail:
tic Coverage: 51.93%
R/base64.R: 0.00%
R/keys.R: 0.00%
R/macro-bookdown.R: 0.00%
R/macro-pkgdown.R: 0.00%
R/mock.R: 0.00%
R/print.R: 0.00%
R/steps-bookdown.R: 0.00%
R/steps-drat.R: 0.00%
R/steps-pkgdown.R: 0.00%
R/steps-ssh.R: 0.00%
R/steps-write-text-file.R: 0.00%
R/use_tic.R: 0.00%
R/ci.R: 36.84%
R/steps-install.R: 42.31%
R/local.R: 46.15%
R/steps-code.R: 46.15%
R/utils.R: 48.00%
R/macro-package-checks.R: 60.71%
R/git2r.R: 66.67%
R/dsl-storage.R: 72.41%
R/steps-rcmdcheck.R: 73.68%
R/stage.R: 85.92%
R/dsl.R: 87.18%
R/run.R: 88.46%
R/steps-git.R: 90.79%
R/repo.R: 90.91%
R/steps-base.R: 100.00%
In general, we require atleast 75% test coverage. Is there a reason coverage is at ~50% and that a number of R/
scripts aren't covered at all?
There is a NOTE that is output from an R CMD check
which is run as part of test-integration-package.R
on a fake package.
#> [36mββ R CMD check results βββββββββββββββββββ ticpkg457a3a5e2a7apkg 0.0.0.9000 ββββ[39m
#> Duration: 3s
#>
#> [34mβ― checking top-level files ... NOTE[39m
#> Non-standard file/directory found at top level:
#> βtic.Rβ
#>
You can safely ignore it. It's just the result of the tic file in the fake package not having been added to the .Rbuildignore
file. I'm just flagging it because I got confused as there's a lot of output from the tests. Thanks @maelle for help trouble shooting!
Also can I draw your attention to the actual test at the end of test-integration-package.R
which is a simple expect_true(TRUE)
and invite you to consider whether a more specific test could be more appropriate?
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 Due date: 2019-06-23
Thanks. I've addressed the editorial comments in v0.2.13.9017:
tic.R
to .Rbuildignore
for internal tests to remove spurious warnings in test output.expect_true(TRUE)
, added a comment.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 notifications@github.com 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.
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
As discussed with editor @annakrystalli, I am the maintainer of the ghactions package for R workflow automation with GitHub Actions.
The package includes all the following forms of documentation:
URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).For packages co-submitting to JOSS
- [ ] The package has an obvious research application according to JOSS's definition
The package contains a
paper.md
matching JOSS's requirements with:
- [ ] A short summary describing the high-level functionality of the software
- [ ] Authors: A list of authors with their affiliations
- [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
- [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).
Estimated hours spent reviewing: 3
CI/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. I checked only a relatively small subset of the feature set, but found the UI/UX pretty smooth and didn't run into any problems.
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. Users should decide for themselves what suits their needs, and time will tell which approaches work, and which won't.
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) *.yml
s used to express workflows on different CI services.
These *.yml
s, after all, are already abstractions over (bash?) scripts run by the services, and are meant as the UI for users.
There is an obvious appeal to abstracting over different providers, and for making this so smooth, but there are also some potential downsides:
*.yml
s by the CI/CD service providers.
(A similar problem appears to be raised in #171)*.yml
, which isn't really that hard.
On the one hand, for easy CI/CD workflows, one can usually just copy/paste a *.yml
(plus some GitHub PAT action, admittedly).
For very custom/complicated CI/CD workflows, on the other hand, learning the R6 class system of tic may be even more complicated than adding the logic of some providers *.yml
.*.yml
s in another abstraction / DSL, one might wonder whether R is, in fact, the right language and level at which to do this.
Arguably, writing service-agnostic workflows could be such a big undertaking (which the tic team seems to have largely covered, incredibly!), that it might be better organised under a "bigger tent" with a larger (non-R) community, that then simply gets wrapped for R convenience.
Absent such a bigger community, I hope that the authors will be able to cope with the maintenance load imposed by this (rapidly?) evolving/consolidating marketplace of tools.*.yml
).
I understand that many R users are not software developers and have limited patience and time for yet more tooling, but by wrapping ever more tools in R, we risk keeping (new) users relatively unprepared for the times when they will need to use outside (standard) tooling.I've learned CI/CD "the hard way", with many wasted hours editing travis.yml
.
Such experiences "in the old days" (well, 2 years go) can easily lead one to falsely that new users should also learn and use tooling this way (cf the base vs tidyverse controversies).
So I might very well be wrong.
But I wonder whether the tradeoff is real. And if the authors agree (that the tradeoff exists), it might be worthwhile adding a bit of a disclaimer to the package for newcomers. I think R users who just quickly want to set up CI/CD should understand that using tic is a choice, and this choice should better be an informed one.
Thank you for your very thoughtful review @maxheld83 ! Stay tuned for @krlmlr's response once all reviews are in.
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
The package includes all the following forms of documentation:
[x] A statement of need clearly stating problems the software is designed to solve and its target audience in README
[x] Installation instructions: for the development version of package and any non-standard dependencies in README
[x] Vignette(s) demonstrating major functionality that runs successfully locally
[x] Function Documentation: for all exported functions in R help
[x] Examples for all exported functions in R Help that run successfully locally
Note: Many help pages didn't have examples. I find examples really useful. Most of the functions that did have examples started with dsl_init()
...and at the moment I'm writing this comment, I'm still confused what is happening (I fully expect that will change by the time I'm done with the review....in case I forget to come back here and edit my comments :) )
[x] Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).
For packages co-submitting to JOSS
- [ ] The package has an obvious research application according to JOSS's definition
The package contains a
paper.md
matching JOSS's requirements with:
- [ ] A short summary describing the high-level functionality of the software
- [ ] Authors: A list of authors with their affiliations
- [ ] A statement of need clearly stating problems the software is designed to solve and its target audience.
- [ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).
Estimated hours spent reviewing: 4
Full disclosure, I am not a usethis
user...it might not be a bad idea for someone who uses and loves that package to review this package. I know it's popular, and I know people really like it, and I get the same vib from this package.
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: https://ropenscilabs.github.io/tic/articles/tic.html
The first thing I did was run tic::use_tic()
...and it was TERRIFYING. I have no idea what I was all authorizing. I was happily answering yes to overwriting local files, but then got really concerned about the github stuff going on (and those authorizations just pop up without a great deal of useful explaination from github what's happening...). I'd say to add more text to the description of the use_tic
help file. I want to have the documentation open to know if I do indeed want to authorize github to do who-knows-what.
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 tic
to ask why something isn't working - they can get back on track quicker, and have a more comprehensive CI setup from the get-go. Risk: when you all are off developing bigger and better packages, you might not have time to answer those users' questions or respond to a breaking API change in Travis or something like that (ie..users get frustrated with tic
vs. getting frustrated with travis... π )
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:
https://blog.methodsconsultants.com/posts/developing-r-packages-with-usethis-and-gitlab-ci-part-iii/
has been incredibly useful to me. If I was already heavily invested in tic
, I'm not sure if I'd have a seamless transition (maybe?).
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 use_tic
function). I really liked the minimal example repos set up to show different use cases.
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!
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.
The first thing I did was run tic::use_tic()...and it was TERRIFYING. I have no idea what I was all authorizing. I was happily answering yes to overwriting local files, but then got really concerned about the github stuff going on (and those authorizations just pop up without a great deal of useful explaination from github what's happening...). I'd say to add more text to the description of the use_tic help file. I want to have the documentation open to know if I do indeed want to authorize github to do who-knows-what.
All steps that are being called by use_tic
are outlined in the help page.
We could add more information on which of these steps trigger the browser windows. The function is already quite verbose and asks you before each step.
Certain tasks require opening browser windows. Would it help if we append this information also in the description of each step so that the user is not surprised by this action?
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 tic to ask why something isn't working - they can get back on track quicker, and have a more comprehensive CI setup from the get-go. Risk: when you all are off developing bigger and better packages, you might not have time to answer those users' questions or respond to a breaking API change in Travis or something like that (ie..users get frustrated with tic vs. getting frustrated with travis... roll_eyes )
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.
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 use_tic function). I really liked the minimal example repos set up to show different use cases.
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.
So for example...adding something right up front like:
You might already know that travis can:
check your package run your tests figure out how well your code is tested
But did you know it could also:
create the man files build the pkgdown site and push those back to your github site (something about drat....I don't quite follow that use-case)
This looks like a good structure that we indeed could implement in the README to get people on board.
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, 100 , 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.
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:
use_tic()
I'm a bit skeptical as to whether it's a good idea to wrap yet another layer on top of the (usually) .ymls used to express workflows on different CI services. These .ymls, after all, are already abstractions over (bash?) scripts run by the services, and are meant as the UI for users.
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. Speaking from the usage side, one has to learn the specific yaml options or discover them in the first place. Of course the same needs to be done for tic if we would compare it with the macros offered by the package. However, the latter scale across CI systems whereas the former do not. And this is in the end the big advantage.
The second big advantage is the simplification of complex tasks. These are made easy by allowing it to be written in R instead of dealing with a shell language (which most of the people are not so familiar with).
There is an obvious appeal to abstracting over different providers, and for making this so smooth, but there are also some potential downsides:
The complexity of managing CI/CD workflows grows considerably; now there's another abstraction layer which may have bugs or unexpected behavior.
True, this will be the case or at least a potential downside that thism pkg comes with.
This will, among other things, require the authors to keep their package up to date with any upstream changes to the *.ymls by the CI/CD service providers. (A similar problem appears to be raised in #171) It's not obvious whether the abstraction (the "steps") actually do or will map elegantly to all providers now or in the future. For a counter-example, GitHub Actions doesn't know "steps." (GitHub Actions can be used for CI/CD, but it's still pretty new and weird, so this isn't a very relevant limitation right now -- and maybe it could be mapped).
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.
An upstream repo which is able to do automatic updates to child-repos containing the pure template file for a specific CI provider would be a nice addition - however I do not see a way how to implement this functionality easily right now (I do not know if this is possible at all while keeping the git
setup behind this somewhat simple).
It's not obvious that the interface of tic is more obvious or user-friendly than writing a *.yml, which isn't really that hard.
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.
On the one hand, for easy CI/CD workflows, one can usually just copy/paste a .yml (plus some GitHub PAT action, admittedly). For very custom/complicated CI/CD workflows, on the other hand, learning the R6 class system of tic may be even more complicated than adding the logic of some providers .yml.
The user is not bothered with R6 as all exported functionality are simple functions and not R6 classes to interact with.
If wrapping *.ymls in another abstraction / DSL, one might wonder whether R is, in fact, the right language and level at which to do this. Arguably, writing service-agnostic workflows could be such a big undertaking (which the tic team seems to have largely covered, incredibly!), that it might be better organised under a "bigger tent" with a larger (non-R) community, that then simply gets wrapped for R convenience.
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).
Absent such a bigger community, I hope that the authors will be able to cope with the maintenance load imposed by this (rapidly?) evolving/consolidating marketplace of tools. Alternatively, it would be interesting to see whether other communities (Python, Julia?) have also developed their own CI/CD abstraction/DSL.
Definitely, I have not checked on this yet.
Lastly, one might wonder whether it's actually a good idea pedagogically, so to speak, to isolate R users from anything non-R (bash scripting, *.yml). I understand that many R users are not software developers and have limited patience and time for yet more tooling, but by wrapping ever more tools in R, we risk keeping (new) users relatively unprepared for the times when they will need to use outside (standard) tooling.
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.
I've learned CI/CD "the hard way", with many wasted hours editing travis.yml. Such experiences "in the old days" (well, 2 years go) can easily lead one to falsely that new users should also learn and use tooling this way (cf the base vs tidyverse controversies). So I might very well be wrong.
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.
But I wonder whether the tradeoff is real. And if the authors agree (that the tradeoff exists), it might be worthwhile adding a bit of a disclaimer to the package for newcomers. I think R users who just quickly want to set up CI/CD should understand that using tic is a choice, and this choice should better be an informed one.
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:
@annakrystalli
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 https://github.com/ropensci/software-review/issues/305#issuecomment-498207401.
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.
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 use_tic()
act smarter and a lot more interactive as before. This should give the user a better idea of what is happening.
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 https://github.com/ropenscilabs/tic/pull/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 https://github.com/ropenscilabs/tic/pull/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?
Hi @annakrystalli
tic is now ready for migration to ropensci from our side.
The CRAN roadmap is as follows:
Regarding tic specifically:
use_tic()
for use in multiple reposWe've done a major refactoring to how use_tic()
works. It now guides the user through a set of questions and finally stores the selected yml templates in the repo as well as taking care for setting up permissions for deployment.
Also users can now single-call use_XY_yml()
to simply add/overwrite their existing templates with the ones provided by tic without having to go through the whole setup process of use_tic()
.
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 devtools::check()
I get this note:
use_tic: no visible global function definition for βcli_alertβ
use_tic: no visible global function definition for βcli_textβ
use_tic: no visible global function definition for βcli_h1β
use_tic: no visible global function definition for βcli_ulβ
use_tic: no visible global function definition for βcli_parβ
use_tic: no visible global function definition for βcli_endβ
use_tic: no visible global function definition for βmenuβ
yesno: no visible global function definition for βmenuβ
Undefined global functions or variables:
cli_alert cli_end cli_h1 cli_par cli_text cli_ul menu
Consider adding
importFrom("utils", "menu")
to your NAMESPACE file.
These functions are from package cliapp
? At the minute, cliapp
is not listed as a dependency, and the functions above don't seem to be imported anywhere. Could you have a little look into it?
@annakrystalli Sorry for that. I missed to add the "cli (>= 2.0.0)" requirement in the DESCRIPTION. Before we only had "cli" there.
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! π I'm trying to test it out with this example bookdown repo: https://github.com/annakrystalli/reprohack-docs
However, now at the end of tic::use_tic()
using selections 2,1,2,2,4 I get:
Finished initiating sync with GitHub.
Waiting for sync with GitHub
Finished sync with GitHub.
Error: You may need to retry in a few seconds. If your repo annakrystalli/reprohack-docs belongs to an organization, you may need to allow Travis access to that organization. Please visit
https://github.com/settings/connections/applications/f244293c729d5066cf27
A browser window will be opened.
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 travis::travis_sync()
.
It is now easy to say what might be the problem here. You could
travis::travis_sync()
manually again and it might resolve itself. Even though this should not be needed.ropenscilabs/travis@travis-com
. We did a complete refactoring of the auth process in this branch and it will soon be merged into master. This might also solve the issue already.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 R_TRAVIS
token is to specify the endpoint
and I set it to .com
it all worked.
However, sadly the resulting build fails
The pertinent error being:
$ R -q -e '{{{install_tic}}}; print(tic::dsl_load()); tic::prepare_all_stages()'
> {{{install_tic}}}; print(tic::dsl_load()); tic::prepare_all_stages()
Error: object 'install_tic' not found
Execution halted
I reproduced this on another repo too
Like 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 travis + tic
vignette and maybe even generalising to include the setup for other CIs too.
Ultimately, users shouldn't have to go to docs in other packages for basic set up instructions for using tic
.
Here's some more specificing suggestions for improvements to documentation and functionality:
[x] Include description of correct travis app installation and permissions configuration on GitHub.
use_tic
).[x] mention the difference between .com
and .org
Travis CI
and Travis CI for Open Source
which could also be confusing).org
to .com
[ ] indeed, perhaps the default R_TRAVIS
should be .com
given the current changes to travis?
[x] what it is used for and how to set up R_TRAVIS
token including an example of setting this up eg using usthis::edit_r_environ()
to add env var to .Renviron
endpoint
during travis::travis_sync()
but this is not available through use::tic()
. Perhaps it could be another question, an argument or some other way you think is most appropriate but I feel users should be able to override the default endpoint
during use::tic()
, at least for now.[ ] In a troubleshooting section, get in front of questions about:
[ ] What do you think about including
use_tic
without necessarily breaking down the questions and then a quick example of what a tic.R
file might look like in the README? This allows code orriented users to get an idea of the workflow at first glance of the repo.
[ ] During travis::travis_sync()
the API token is pasted in the console and (although deleted form history) is still visible in the console. Could this be avoided in any way?
[ ] When there is no github repository, I just get:
Error: Failed to lookup git remotes
Perhaps a more informative message, prompting the user to running (or even running) usethis::use_github(protocol = "https")
(if using a PAT)
[ ] I also noticed that I can run dsl_init()
on repositories I have not initiated with tic
. While running further functions (eg do_bookdown()
) after that returns a non informative error. It would be better for dsl_init()
to check whether the project is initialised with tic.R
before trying to run macros.
[x] Docs url https://docs.ropensci.org/tic/dev resolves to a 404 page
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? Or are there just some of these (if at all) which you consider blocking for the migration?
(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).
The build fails
The pertinent error being:
$ R -q -e '{{{install_tic}}}; print(tic::dsl_load()); tic::prepare_all_stages()'
{{{install_tic}}}; print(tic::dsl_load()); tic::prepare_all_stages() Error: object 'install_tic' not found Execution halted
Thanks for catching this. Replacing {{{install_tic}}}
with remotes::install_github("ropenscilabs/tic")
solves this. I've also updated the templates already.
OK thanks.
I've updated .travis.yml
and pushed. Now I'm getting
> tic::install()
β Loading tic stage configuration from tic.R
Running install: step_install_deps(repos = repo_default())
<error/rlang_error>
cannot open the connection
Backtrace:
1. tic::install()
2. tic::run_stage("install", stages = stages)
3. stage$run_all()
4. private$run_one(step)
11. step$run()
14. remotes::install_deps(...)
15. remotes::dev_package_deps(...)
16. remotes:::load_pkg_description(pkgdir)
17. remotes:::read_dcf(path_desc)
20. base::read.dcf(path)
Error: A step failed in stage "install": install.
In addition: Warning message:
In read.dcf(path) :
cannot open compressed file '/home/travis/build/annakrystalli/reprohack-docs/DESCRIPTION', probable reason 'No such file or directory'
Execution halted
The command "R -q -e 'tic::install()'" failed and exited with 1 during .
I'm just using tic vanilla bookdown setup https://github.com/annakrystalli/reprohack-docs. Is a DESCRIPTION file necessary for this to work?
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. See also the bookdown example repo for more info. I think the default error message is quite descriptive here, what do you think?
SUCCESS!! π
So, If the project requires it, I feel do_bookdown()
should check for a DESCRIPTION
file and warn earlier rather than wait to get an error on TRAVIS. Indeed the vanilla new bookdown project (created through Rstudio) which is how I set up the test repo, does not come with a DESCRIPTION
file.
If I try and use usethis::use_description()
to get a starting template, I get an error because the repo name reprohack-docs
has non ascii character (which might be typical for non package projects). I ended up using holepunch::write_compendium_description()
to create a DESCRIPTION
file but it might be nice to have similar functionality within tic
and create a DESCRIPTION
file if it's missing during use_tic
, given that it's effectively a dependency for any tic
project.
Indeed, if tic
is guessing which type of project it is, it could also check to make sure known required dependencies are specified (eg markdown, bookdown etc). it should also be mentioned in function/general docs that a DESCRIPTION
file is required and should be used to track dependencies. Might be good to mention that missing dependencies in the DESCRIPTION
file will result in errors on travis.
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 tic
until the version of travis
I had to install is merged into master. Do you have any idea when that might be?
So, If the project requires it, I feel do_bookdown() should check for a DESCRIPTION file and warn earlier rather than wait to get an error on TRAVIS. Indeed the vanilla new bookdown project (created through Rstudio) which is how I set up the test repo, does not come with a DESCRIPTION file.
I see. We can check for this during use_tic()
but not within do_bookdown()
. The latter is a macro function and is only being executed on the CI system (unless the user emulates the CI run locally). However, it does not have any setup/check functionality before starting a run.
I've created an issue to better assert the existence of a DESCRIPTION file.
I also can't accept tic until the version of travis I had to install is merged into master. Do you have any idea when that might be?
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 travis-com
branch.
Could we quickly summarize which issues are blocking for the transition? I'd like to outsource your points from https://github.com/ropensci/software-review/issues/305#issuecomment-565804724 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.
In general, {tic} should also work with the current state of the {travis} package, even though it is limited to the .org endpoint
The problem with that is that since May 2018, travis has announced a movement towards .com
so I'd rather not accept tic
without .com
endpoint functionality.
Include description of correct travis app installation and permissions configuration on GitHub.
There is only one app left that can be actively installed. I written some sentences about this in https://github.com/ropenscilabs/tic/pull/213
mention the difference between .com and .org
Done in https://github.com/ropenscilabs/tic/pull/213
indeed, perhaps the default R_TRAVIS should be .com given the current changes to travis?
Done in https://github.com/ropenscilabs/travis/commit/a8b7ad37ad1a56fcdc34f03d542bead248decd18
what it is used for and how to set up R_TRAVIS token including an example of setting this up eg using usthis::edit_r_environ() to add env var to .Renviron
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 R_TRAVIS
at https://docs.ropensci.org/travis/articles/travis.html#setting-the-default-endpoint.
This could be cumbersome to work with if you currently have repositories split across both. Currently overriding it for individual repositories is possible through specifying the endpoint during travis::travis_sync() but this is not available through use::tic(). Perhaps it could be another question, an argument or some other way you think is most appropriate but I feel users should be able to override the default endpoint during use::tic(), at least for now.
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 endpoint
arg in use_tic()
: https://github.com/ropenscilabs/tic/pull/213
In a troubleshooting section, get in front of questions about:
- common setup errors - setup for private repositories. - setup for orgs
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. Regarding the other two points: There is no difference for these scenarios. If so, issues should go to the API client packages {travis} and {circle}.
without necessarily breaking down the questions and then a quick example of what a tic.R file might look like in the README? This allows code orriented users to get an idea of the workflow at first glance of the repo.
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 ?tic::use_tic()
helps. I am not sure to what extent a c/p tic.R in the README would add added value compared to all the contents in the already existing vignettes. The vignettes break down how {tic} works, what steps/stages are and that everything comes together in the tic.R
file. Is there something you miss in all of these? Or could you maybe clarify more what the advantage of this action would be so I can follow?
When there is no github repository, I just get:
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.
I also noticed that I can run dsl_init() on repositories I have not initiated with tic. While running further functions (eg do_bookdown()) after that returns a non informative error. It would be better for dsl_init() to check whether the project is initialised with tic.R before trying to run macros.
{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 use_tic()
first if someone calls dsl_init()
without a tic.R
file (https://github.com/ropenscilabs/tic/pull/213)
I also can't accept tic until the version of travis I had to install is merged into master. Do you have any idea when that might be?
The {travis} refactoring is done.
Docs url docs.ropensci.org/tic/dev resolves to a 404 page
We will stick with https://docs.ropensci.org/tic until we are on CRAN.
dsl_init()
doesn't require a tic.R
file. In https://github.com/ropenscilabs/tic/pull/213 we have adapted the output after dsl_init()
and macro functions such as do_bookdown()
to include a link to a help page.
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).
Hello @pat-s and @krlmlr,
Thanks for all your work! I feel all my comments have been addressed apart from the following:
what it is used for and how to set up R_TRAVIS token including an example of setting this up eg using usthis::edit_r_environ() to add env var to .Renviron
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 R_TRAVIS at https://docs.ropensci.org/travis/articles/travis.html#setting-the-default-endpoint.
Because this environmental variable needs to be set for tic
to work with travis
, it should also be mentioned in tic
documentation. It's ok to just mention that it is required in the tic
help file and getting started vignette and link to the travis
vignette for more details if you prefer. But the tic
user needs to know about it upfront. And remember, for documentation the opposite principle to code applies of do repeat yourself π
The rest are just some responses to some of your comments @pat-s and general suggestions but no action is necessary to pass review.
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.
As a user that tried to set up tic
for docs in an organisation repo, I will have to disagree. Googling error messages was not fun and it took me ages to figure out how to set up authorisations and apps properly. The difficulties arise from how GitHub works (and their poor documentation IMHO) in that there are setting that need to be made on the org level AND on the user level, and the existence of the two endpoints and therefore travis apps on older users accounts further complicates the issue. So while it is not tic
's deficiency in any way and I will not hold up the review for this, my suggestion still stands to provide such information for users, ie at the very least, how to set up tic
for an org repo.
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 ?tic::use_tic() helps. I am not sure to what extent a c/p tic.R in the README would add added value compared to all the contents in the already existing vignettes. The vignettes break down how {tic} works, what steps/stages are and that everything comes together in the tic.R file. Is there something you miss in all of these? Or could you maybe clarify more what the advantage of this action would be so I can follow?
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 tic.R
file rather than walking through tic code, just to hammer through that this is code that lives in this file and is not executed in the console. But just ignore this suggestion.
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.
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:
In ropenscilabs/tic#213 we have adapted the output after dsl_init() and macro functions such as do_bookdown() to include a link to a help page
That's great.
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). I don't have a strong opinion about this so will defer to what you think is best.
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 ?use_tic
help page in https://github.com/ropenscilabs/tic/pull/213/commits/1bc46995dd26de5459e29e1650ff582a0e095108. I agree that this is out of scope for the package and we should provide helpful links. This form will be picked up by pkgdown's autolinker -- users of the pkgdown website will see a direct link to the relevant vignettes.
Approved! π₯³
Thanks @krlmlr & @pat-s for submitting and @ldecicco-USGS & @maxheld83 for your reviews! πΈ
To-dos:
[![ropensci_footer](https://ropensci.org/public_images/ropensci_footer.png)](https://ropensci.org)
"pkgdown
website, fix its URL to point to https://docs.ropensci.org/package_name
and deactivate the automatic deployment you might have set up, since it will not be built centrally like for all rOpenSci packages, see http://devdevguide.netlify.com/#docsropensci. 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
DESCRIPTION
via rodev::add_ro_desc()
.[![AppVeyor Build Status](https://ci.appveyor.com/api/projects/status/github/ropensci/pkgname?branch=master&svg=true)](https://ci.appveyor.com/project/individualaccount/pkgname)
.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 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 βΊοΈ. Just let @stefaniebutland know if you are interested.
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
DESCRIPTION
fileScope
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
- [ ] 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
CC @pat-s @maelle @karthik