Ensure that mode.getFullVersion_ falls back to the full version and not just major #5946
Conversation
dvoytenko
changed the title
|
@jridgewell for final review. not sure if this affects sw in any way |
| // We will default to production default `01` minor version for now. | ||
| // TODO(erwinmombay): decide whether $internalRuntimeVersion$ should contain | ||
| // minor version. | ||
| return `01${version}`; |
There was a problem hiding this comment.
01 may not always be the Prod prefix, though. I think. @cramforce would know better than I would.
There was a problem hiding this comment.
Yes, 01 is not always the best. But it should always be there. And we have no better information for a fallback at this time. Separately, I asked @erwinmombay to consider to simply update $internalRuntimeVersion$ to a full version or add another replacement like this for minor version.
There was a problem hiding this comment.
I took to using two types, AmpVersion and RtvVersion in the SW to help catch issues where I used a non-prefixed value in a place that requires a prefix. Maybe we should do the same here to catch these kinds of issues?
There was a problem hiding this comment.
Something like rtvVersion definitely makes sense.
There was a problem hiding this comment.
Ok. I added rtvVersion field to disambiguate the two.
|
LGTM. |
…ot just major (ampproject#5946) * Ensure that mode.getFullVersion_ falls back to the full version and not just major * renames * types * update rtvVersion everywhere
…ot just major (ampproject#5946) * Ensure that mode.getFullVersion_ falls back to the full version and not just major * renames * types * update rtvVersion everywhere
Partial for #5940.