Define USER and BRANCH in the Docker image#393
Define USER and BRANCH in the Docker image#393mkhansenbot merged 3 commits intoros-navigation:masterfrom
Conversation
mkhansenbot
left a comment
There was a problem hiding this comment.
Thank you for sumitting this PR, your contribution is appreciated, however, I would like a minor change to how you're doing the if conditional. See my inline comment.
Dockerfile
Outdated
| ARG PULLREQ=false | ||
| ARG BRANCH=master | ||
| RUN echo "pullreq is $PULLREQ" | ||
| RUN if [ "$PULLREQ" == "false" ]; \ |
There was a problem hiding this comment.
I'd prefer if you made if [ $PULLREQ == "false" && $BRANCH == "master"]; then (echo the message) be the base case, but add a new case for elif [ $BRANCH != "master" ]; then checkout the branch.
I realize the end effect is the same but it makes the default case do nothing instead of checking out master onto a temporary branch which is more clear, I think it also is more readable and understandable in the $BRANCH case.
mkhansenbot
left a comment
There was a problem hiding this comment.
Thanks for making the change. Looks good to me 👍
@crdelsey or @SteveMacenski - please review
| cd -; \ | ||
| echo "No pull request number given - defaulting to $BRANCH branch"; \ | ||
| else \ | ||
| git fetch origin pull/$PULLREQ/head:pr_branch; \ |
There was a problem hiding this comment.
This last else clause gets executed if PULLREQ is true and BRANCH is master. Is that possible?
There was a problem hiding this comment.
BRANCH is master by default, so I would expect that to be the case when PULLREQ is true.
Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Basic Info
Description of contribution in a few bullet points
It is the response to the discussion in #389. I updated Travis, so it errors if it runs in any repo but ros-planning/navigation2. Added retries for
wget. For some reasons, I cannot download the entire archive without retries, probably due to my physical location. The other changes are about the ability to customize GitHub username and branch while running Docker. The default has not been changed.Hope this is a better PR than the previous one.
@SteveMacenski, @mkhansen-intel