Skip to content

runtime-version: Update for custom hosts and lts#695

Merged
sebastianbenz merged 3 commits intoampproject:masterfrom
mdmower:pr-rvupdate
Apr 15, 2020
Merged

runtime-version: Update for custom hosts and lts#695
sebastianbenz merged 3 commits intoampproject:masterfrom
mdmower:pr-rvupdate

Conversation

@mdmower
Copy link
Copy Markdown
Contributor

@mdmower mdmower commented Apr 5, 2020 •
edited
Loading

  • Support lts flag, both as an argument to .getVersion() and as a
    CLI argument.
  • Fall back to /version.txt if /rtv/metadata is not available.
    This feature is only available for standard prod lookups, it
    is not used when a canary or lts release is requested. This
    lessens the requirements for self-hosting the runtime.
  • Remove RTV padding. There's little chance that padding
    the result is going to ever result in a valid RTV.
  • Gracefully return undefined (or empty string in CLI) when version
    is not available.
  • Update test spec to use fetch-mock so network requests are not made.

@mdmower
runtime-version: Update for custom hosts and lts
8af9a61 
* Prefer the /rtv/metadata endpoint if available, but fall back to
  constructing an RTV from the release type (prod, canary, or lts)
  code and the version, available from /version.txt. This broadens
  support for self-hosted runtimes. If /rtv/metadata data is available,
  continue trusting it without additional validation. If the version is
  retrieved from /version.txt, then ensure /rtv/<rtv>/version.txt also
  exists before assuming it is valid.
* Remove unnecessary RTV padding. There is nothing in amphtml that
  prevents a version from being defined by
    gulp dist --version_override="v1.2a"
* Support lts flag, both as an argument to .getVersion() and as a CLI
  argument.
* Update test spec to use fetch-mock so network requests are not made.
@mdmower
Copy link
Copy Markdown
Contributor Author

mdmower commented Apr 5, 2020 •
edited
Loading

Here are some samples of runtime version lookups supported by this PR:

AMP Project LTS

$ packages/cli/bin/amp runtime-version --lts
> 012002251816300

Self-hosted runtime with custom version string

$ packages/cli/bin/amp runtime-version --host "https://ampdemo.cmphys.com/amp-rt"
> 01v1.2a

Self-hosted canary runtime with custom version string

$ packages/cli/bin/amp runtime-version --host "https://ampdemo.cmphys.com/amp-rt" --canary
> 00v1.2a

Bing does not have /rtv/metadata, falls back to version.txt lookup

$ packages/cli/bin/amp runtime-version --host "https://www.bing-amp.com"
> 011912201827130

Bing does not have canary releases and version.txt lookup doesn't support them anyways

$ packages/cli/bin/amp runtime-version --host "https://www.bing-amp.com" --canary
>

@mdmower mdmower mentioned this pull request Apr 5, 2020
Merged
@mdmower
Copy link
Copy Markdown
Contributor Author

mdmower commented Apr 5, 2020

With respect to bullet point "Remove unnecessary RTV padding. There is nothing in amphtml that
prevents a version from being defined by...", I've decided to open a question at amphtml: ampproject/amphtml#27579

@mdmower
Copy link
Copy Markdown
Contributor Author

mdmower commented Apr 11, 2020

With respect to bullet point "Remove unnecessary RTV padding. There is nothing in amphtml that
prevents a version from being defined by...", I've decided to open a question at amphtml: ampproject/amphtml#27579

We only have to worry about 15-digit RTVs. I still question any benefit of padding a result from /rtv/metadata that isn't 15-digits. It is very unlikely that padding would identify a "correct" version. It seems sensible to remove the padding, so I'm leaving the PR as is.

@sebastianbenz
Copy link
Copy Markdown
Collaborator

sebastianbenz commented Apr 14, 2020

We only have to worry about 15-digit RTVs. I still question any benefit of padding a result from /rtv/metadata that isn't 15-digits. It is very unlikely that padding would identify a "correct" version. It seems sensible to remove the padding, so I'm leaving the PR as is.

The padding is a leftover from the time before we had a /rtv/metadata endpoint.

sebastianbenz
sebastianbenz approved these changes Apr 14, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@sebastianbenz sebastianbenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! One nit.

packages/runtime-version/lib/RuntimeVersion.js Outdated
let response;
try {
response = await this.fetch_(versionTxtUrl);
} catch (ex) {}
Copy link
Copy Markdown
Collaborator

@sebastianbenz sebastianbenz Apr 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a comment explaining why we can ignore the exception

Copy link
Copy Markdown
Contributor Author

@mdmower mdmower Apr 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added. Thanks for reviewing!

@sebastianbenz sebastianbenz merged commit b9d9a34 into ampproject:master Apr 15, 2020
@sebastianbenz
Copy link
Copy Markdown
Collaborator

sebastianbenz commented Apr 15, 2020

Thanks!

@mdmower mdmower deleted the pr-rvupdate branch October 9, 2022 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sebastianbenz sebastianbenz sebastianbenz approved these changes

Assignees

No one assigned

Labels

cla: yes enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mdmower @sebastianbenz @googlebot