deislabs/hippo-cli

Auto-generated pre-release info may not be valid semver

vdice opened this issue ยท 5 comments

vdice commented

The auto-generated pre-release information attached to a hippo app's bindle version by this CLI may not be valid semver.

Specifically, if there are components in the pre-release data with leading zeros, the semver library/crate used by bindle will return an error. (See dtolnay/semver#230, the fix for which has been in the aforementioned library since its 1.0.0 release and has been running in bindle since mid-September.)

As an example, the version 0.1.0-vdice-2021.11.08.11.43.43.548 was produced earlier today via hippo prepare. The problematic component in this example is the 08 portion.

Attempting to push the bindle with this version results in:

$ hippo push -k .
Error: Error pushing bindle to server: Invalid request (status code 400): Some("Request body toml deserialize error: invalid leading zero in pre-release identifier for key `bindle` at line 21 column 1")
vdice commented

Wanted to toss around ideas for possible fixes...

  1. Keep the . separators between the pre-release info (specifically, the date/time components) but forbid any component to lead with a zero. It sounds like we may need to implement custom sorting if we go this route.
  2. Use a hyphen separator instead of .. I'm having success pushing bindles with versions like 0.1.0-vdice-2021-11-09-08-19-36-007.
  3. Instead of time/date info, perhaps consider using a ULID. Sortable, but opaque.

RE option 2: according to semver's grammar reference, only one hyphen is considered valid, so the example you provided wouldn't work if we're following semver properly. I've included the relevant sections of the backus-naur.

<valid semver> ::= <version core>
                 | <version core> "-" <pre-release>
                 | <version core> "+" <build>
                 | <version core> "-" <pre-release> "+" <build>

<version core> ::= <major> "." <minor> "." <patch>

<pre-release> ::= <dot-separated pre-release identifiers>

<dot-separated pre-release identifiers> ::= <pre-release identifier>
                                          | <pre-release identifier> "." <dot-separated pre-release identifiers>

<pre-release identifier> ::= <alphanumeric identifier>
                           | <numeric identifier>

note: alphanumeric identifiers do allow one hyphen (foo-bar), but it also states that they must be dot-separated pre-release identifiers, so foo-bar-2021-11-09-08 doesn't appear to be valid while foo-bar.2021-11.09-08 might be... But that seems a little arbitrary/counter-intuitive.

vdice commented

@bacongobbler how about this: we remove all separators within the pre-release info (timestamp)? We lose a bit of human-readability, but we maintain sortability and semver compliance. For example:

0.1.0-vdice-20211115080018993

$ git diff
diff --git a/src/expander.rs b/src/expander.rs
index aa2ba52..44dc2fd 100644
--- a/src/expander.rs
+++ b/src/expander.rs
@@ -41,7 +41,7 @@ impl ExpansionContext {
                     .map(|s| format!("-{}", s))
                     .unwrap_or_else(|| "".to_owned());
                 let timestamp = chrono::Local::now()
-                    .format("-%Y.%m.%d.%H.%M.%S.%3f")
+                    .format("-%Y%m%d%H%M%S%3f")
                     .to_string();
                 format!("{}{}{}", version, user, timestamp)
             }

That sounds good to me @vdice - simple to implement, practical and readable enough if anyone ever needs to.