[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #300: Use a container for building release candidates

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #300: Use a container for building release candidates
URL: https://github.com/apache/incubator-daffodil/pull/300#discussion_r357392755
 
 

 ##########
 File path: containers/release-candidate/daffodil-release-candidate
 ##########
 @@ -0,0 +1,255 @@
+#!/bin/bash
 
 Review comment:
   The reason for the name change is pretty minor. The instructions for building the container say to use the container name of "daffodil-release-candidate", which is a reasonable and sufficiently unique name to differentiate from other containers on the system.
   
   When you run this container with that name, e.g.
   
       podman run ... daffodil-release-candidate
   
   any arguments you provide after that are provided to the entrypoint script. So for example you can run this to do a test build:
   
       podman run ... daffodil-release-candidate --dry-run
   
   And the entrypoint script will change its behavior so it doesn't publish anything. If you give bad arguments or the --help option, then it will print the usage. But the usage displays the name of the script, not the name of the container (the script doesn't even know it's in a container). By giving the entrypoint the same name as the name as the container, the usage just looks more like what someone might expect and almost feels like you're not even running in a container, e.g.:
   
       podman run ... daffodil-release-candidate --help
       usage: daffodil-release-candidate [OPTION]...
       Options:
         -n, --dry-run   Run all commands but do not actually publish anything
         -h, --help      Display this help and exit
   
   If the script name were something different, it wouldn't match the container name and the usage might be slightly confusing. Granted, you can use whatever name you want for the container, so it won't necessarily match the script name, but that's the container name suggested in the readme so that it does match.
   
   Same reasoning for dropping the .sh. Purely just so the container name matches the script name in the usage.
   
   I don't feel strongly about this if it seems unnecssary.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services