benmarwick / rrtools

rrtools: Tools for Writing Reproducible Research in R
Other
670 stars 85 forks source link

Travis build using docker fails if compendium name contains capital letters #98

Closed annakrystalli closed 4 years ago

annakrystalli commented 4 years ago

Please wait for some discussion of your report before making a Pull Request.

Describe the bug

The default specification of variable $REPO in the travis.yml created using rrtools::use_travis(docker=T) causes the build to fail on TRAVIS because of an invalid tag format if the compendium name contains capital letters. Logs return the following error so it seems that docker hub does not accept capitals in tags.

invalid argument "[secure]/rrcompendiumRSE19" for t: invalid reference format: repository name must be lowercase
See 'docker build --help'.

This could be problematic as compendium naming is already restricted so not allowing capitals in compendium names is not ideal.

To Reproduce Here's the logs of a TRAVIS build of a compendium showing the error, with the travis.yml in the default state.

Expected behavior

I fixed the error by assigning an all lower case version of $REPO in my .travis.yml as such: REPO="$(echo $DOCKER_USER/rrcompendiumRSE19 | tr A-Z a-z)"

See full .travis.yml that works.

Would be great to include this tweak in the template as it took me a couple of hours to figure out and fix and may well be an issue many end up encountering. Perhaps as:

REPO="$(echo $DOCKER_USER/{{{repo}}} | tr A-Z a-z)"

in the travis.yml-with-docker template.

(beware REPO is defined twice in the template)

Look forward to your thoughts

benmarwick commented 4 years ago

Yes, right, I see what you mean. This came up in https://github.com/benmarwick/rrtools/issues/76, so I think we just undo that change and should fix this.

benmarwick commented 4 years ago

Looking over the discussion in #76, I'd like to follow up on the option to separate the package name from the repo name. For example @cboettig names his compendium pkgs simply 'compendium', and then has more flexible names for the repo: https://github.com/cboettig/noise-phenomena/blob/master/DESCRIPTION

I like that because it saves the user from making a decision they probably don't care about. CRAN is not the end point for these compendium pkgs, so it hardly matters what the pkg is called.

The proposal to move the name checking to use_travis doesn't seem ideal because by then if we find we have to change it then we have to change it in many places.

What do you think about that @nevrome @wolass @adamhsparks ?

annakrystalli commented 4 years ago

It's important to note that what's complaining here is Docker and in particular trying to build an image that includes upper case letters in the tag. GitHub is fine with capitals and as a default behaviour I think repo name should reflect compendium name.

The solution I'm proposing is to just ensure the docker image is tagged with the repo name in all lower case characters and that that's handled automatically in the TRAVIS file. So folks will still be free to have upper case in their compendium names and GitHub repos. The name of the image on Dockerhub will be the only thing that's slightly inconsistent (ie all lowercase).

Sent from my iPhone

On 13 Sep 2019, at 01:51, Ben Marwick notifications@github.com wrote:

Looking over the discussion in #76, I'd like to follow up on the option to separate the package name from the repo name. For example @cboettig names his compendium pkgs simply 'compendium', and then has more flexible names for the repo: https://github.com/cboettig/noise-phenomena/blob/master/DESCRIPTION

I like that because it saves the user from making a decision they probably don't care about. CRAN is not the end point for these compendium pkgs, so it hardly matters what the pkg is called.

What do you think about that @nevrome @wolass @adamhsparks ?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

annakrystalli commented 4 years ago

Actually I thought of a really simple and small fix that could address this issue. Have a little look at #100 and let me know what you think. Worked for my compendium! 😃

benmarwick commented 4 years ago

Let's try it! I know you have a workshop very soon (and I'm doing another one in Japan today)

annakrystalli commented 4 years ago

I can write a test too if you like.