nhs-england-tools / repository-template

🛠️ 📚💡 This is a detailed and carefully made template for your GitHub projects. It's based on the wide knowledge and practical experience of the engineering community within NHS England. The template includes helpful suggestions, standards and practices - it's something you should consider using for all your repositories.
MIT License
30 stars 12 forks source link

Template strings in VERSION are likely to collide with real tags #113

Closed regularfry closed 1 year ago

regularfry commented 1 year ago

What is the problem this feature will solve?

Right now the template substitution done to the VERSION file uses short template strings composed only of letters. One of the goals of the tagging mechanism is that people should be able to use whatever static strings they like to tag images, but with letter-based template tags (especially if people want to name things after branches) we run into this situation:

fixed-hash-function-branch => fixed-1234abcd-function-branch
remove-comments => remove-co08ents
bug-ridden-no-more => bug-ri30en-no-more

...among other possible examples.

What is the feature that you are proposing to solve the problem?

We replace the use of yyyy/mm/dd template strings with date-format-style specifiers, implemented either explicitly, by substituting %Y/%m/%d-style template tags with sed as now, or implicitly, by passing the entire line through to date itself as a format string. In the latter case we'd need to come up with a way to handle git hashes and anything else we want to expose.

What alternatives have you considered?

No response

Code of Conduct

Sensitive Information Declaration

stefaniuk commented 1 year ago

Good spot. Thanks. I propose we use the ${yyyy}${mm}${dd}${HH}${MM}${SS}-${hash} pattern instead of yyyymmddHHMMSS-hash. Therefore, the sed expressions change to:

cat "$dir/VERSION" | \
      sed "s/\${yyyy}/$(date --date="${build_datetime}" -u +"%Y")/g" | \
      sed "s/\${mm}/$(date --date="${build_datetime}" -u +"%m")/g" | \
      sed "s/\${dd}/$(date --date="${build_datetime}" -u +"%d")/g" | \
      sed "s/\${HH}/$(date --date="${build_datetime}" -u +"%H")/g" | \
      sed "s/\${MM}/$(date --date="${build_datetime}" -u +"%M")/g" | \
      sed "s/\${SS}/$(date --date="${build_datetime}" -u +"%S")/g" | \
      sed "s/\${hash}/$(git rev-parse --short HEAD)/g" \
    > "$dir/.version"

The ${variable} pattern used for substitution should be recognised by anyone who is familiar with shell/make scripting.

regularfry commented 1 year ago

That's probably fine, but there's a little voice in the back of my head saying that if it looks like shell variables we'll get people trying to use $yyyy without the brackets, and getting confused when that doesn't work. We could make the brackets optional?

stefaniuk commented 1 year ago

Added but not documented as the use of it is discouraged due to readability: ${yyyy}${mm}${dd} vs. $yyyy$mm$dd

regularfry commented 1 year ago

I like it, that's a pretty good tradeoff. We're not encouraging it but we're also not surprising someone whose muscle memory leaves off the brackets.