cli: show release version with --version by taiki45 · Pull Request #2276 · envoyproxy/envoy

taiki45 · 2017-12-28T09:16:48Z

Description:
Fixes #2239

Show release version with --version. The release version shows the last released version.

Before:

version: 92a31b6f67d06078653c75645126639578c266cc/Modified/DEBUG

After:

version: 1c61b12055ff3e4138daa91ccff8bc7cef7bc866/1.5.0/Modified/DEBUG

Other possible implementations are:

Place VERSION file conitaning a release version string like 1.5.0 and use bazel's genrule to generate C++ header file.
Place VERSION file and use bazel's get_workspace_status to generate C++ header file. Currently bazel seems to support only specific keys, so we have to change the behavior. I don't know the change is reasonable for bazel.

Other considerations:

Which should we show a release version (e.g. 1.5.0) or a development version (e.g. 1.6.0.dev) for non-release builds?

Risk Level: Low
I think this is small optional feature.

Testing:
Automated testing should cover this. Also I checked manually too.

Docs Changes:
N/A

Release Notes:
N/A

Fixes #2239

The release version shows the last released version. Before: version: 92a31b6f67d06078653c75645126639578c266cc/Modified/DEBUG After: version: 1c61b12055ff3e4138daa91ccff8bc7cef7bc866/1.5.0/Modified/DEBUG Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

htuch

Thanks for taking this on @taiki45, this is a useful thing to have. Some comments.

htuch · 2017-12-28T11:13:16Z

test/exe/show_version_test.sh

+#!/bin/bash
+set -e -o pipefail
+
+source/exe/envoy-static --version |grep -E '[0-9]+\.[0-9]+\.[0-9]+'


You can probably get the version from a genrule pointing at https://github.com/envoyproxy/data-plane-api/blob/master/docs/root/intro/version_history.rst (with grep/head/sed), this input would come from the @envoy_api//docs/root/intro:version_history.rst input.

Or, you could at least change this test to validate consistency. As it is today, we have two places in the envoy and data-plane-api changes that version is maintained after this PR, would be ideal to have single-source-of-truth or a test to keep them consistent.

Oh, I also found docs/conf.py in data-plane-api also has the version string (1.6.0 there).

Considering the above and #2276 (comment), we should have single version string at VERSION file in data-plane-api repo, and make use of it at docs/conf.py and VersionInfo object. For version_history.rst and RAW_RELEASE_NOTES.md, since it's difficult to embed the version string from VERSION file, we can just write tests which validate consistency.

If it looks good, I can work for that. My concern is it's too complex or not for the task. Should we just hard code the version in a few places?

Also I suggest the version string will be next release version + .dev (e.g. 1.6.0.dev now) to indicate "the git SHA doesn't match the original SHA for the release version".

Want any comments.

I think this is the best solution, go for it.

Yes +1. Let's move the version out into a dedicated file, and read into docs conf, etc. Thank you!!!

htuch · 2017-12-28T11:13:58Z

source/common/common/version.cc


 std::string VersionInfo::version() {
-  return fmt::format("{}/{}/{}", revision(), revisionStatus(),
+  return fmt::format("{}/{}/{}/{}", revision(), ReleaseVersion, revisionStatus(),


For builds in which the git SHA doesn't match the original SHA for the release version, we could also consider appending some suffix to indicate, e.g. 1.5.0+.

BTW, to figure out if we're at the same SHA as the release version, you can do something like git rev-list -n 1 v1.5.0, i.e. make use of the repo tags.

This would all need to be done somewhere like https://github.com/envoyproxy/envoy/blob/2fa90d22521376a7990196e34a1e1bb945663ab4/bazel/get_workspace_status.

It's good idea to indicate that with some suffix. I'm also willing to make use of get_workspace_status but it only accept specific keys as I wrote in the PR description. There might be some possible ways:

hard code the version number with suffix when we're developing, then drop the suffix when we release. Pros: easy and simple implementation. Cons: we need bump the version number after releases.

use genrule. Extract version number string from C++ header file and place somewhere else. Then extract it and pass to git-rev-list to figure out the SHA is same as the release version, generate a number string with suffix or without suffix and place that into C++ header file so that Envoy can use it. Pros: don't need to bump after releases. Cons: complex.

Comparing (1) and (2), the pros of (2) is limited here, so I prefer (1) for now. WDYT?

With some considerations, I suggested a new way so please just ignore the above comment.

With get_workspace_status, you can overload the various fields, but I prefer the alternative you suggest below.

This reverts commit db99351. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Check the proper Envoy version string exists in RAW_RELEASE_NOTES.md. NOTE: this test requires adding release version string into RAW_RELEASE_NOTES.md in future release tasks. Also, when publish docs, check the git version and document version. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

taiki45 · 2018-01-05T02:12:13Z

Implemented introducing and useing VERSION file way with
df715bc and 3a84f89 and envoyproxy/data-plane-api#394.

Probably, we don't need test/version_consistency_test.sh?

htuch

Great, this looks like where we want to be going with this, thanks!

htuch · 2018-01-05T11:35:10Z

RAW_RELEASE_NOTES.md

 final version.

-## 1.6.0
+## 1.6.0.dev


Not sure this is necessary, the release notes are intended for the next tagged version, do no ned for the .dev suffix.

I agree. This is my fault. Just drop dev suffix.

htuch · 2018-01-05T11:35:56Z

docs/publish.sh

 git checkout "${GIT_INFO[1]}"
+# Check the git tag matches the version number in the VERSION file.
+VERSION_NUMBER=$(cat VERSION)
+if [ "v${VERSION_NUMBER}" != $CIRCLE_TAG ]; then


"${CIRCLE_TAG}"

htuch · 2018-01-05T11:39:29Z

test/exe/version_out_test.sh

+#!/bin/bash
+set -e -o pipefail
+
+source/exe/envoy-static --version | grep --fixed-strings $(cat external/envoy_api/VERSION)


I kind of worry that this will be too specific to Bazel tree layout. If you want to keep it, we can see how it goes and remove later, but I don't feel it's that necessary.

Yeah, I can now feel this is too specific for the test. Instead, I just use more loosen specification for this test at 6c21077 (and d4526c2). Is it near the one you thought? I'm not pretty sure what you worried about.

htuch · 2018-01-05T11:39:43Z

test/version_consistency_test.sh

+#!/bin/bash
+set -e -o pipefail
+
+grep --fixed-strings $(cat external/envoy_api/VERSION) RAW_RELEASE_NOTES.md


Yeah, you can skip this one.

+1 26d9125

htuch · 2018-01-05T11:42:30Z

source/common/common/version.h

 namespace Envoy {
+
+// Release version number (e.g. 1.6.0).
+static const std::string& VERSION_NUMBER = BUILD_VERSION_NUMBER;


This is a global non-POD, which is not allowed in Envoy style, see point on " non-PoD static and global variables are forbidden" in https://github.com/envoyproxy/envoy/blob/master/STYLE.md#deviations-from-google-c-style-guidelines.

Is defining this necessary? Can't we just use the C string from BUILD_VERSION_NUMBER everywhere directly?

Oops, I missed the style guide. I think we can use the BUILD_VERSION_NUMBER everywhere direcly. d6b7047

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

We don't need this. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Follow-up of 6c21077. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

mattklein123

this is awesome, thanks!

I believe envoyproxy/envoy-mobile#2059 helped reduce these occurrences, but we're still seeing some termination crashes where a different thread is waiting on `std::thread::join()`. So moving this to before the `main_thread_.join()` may help. Risk Level: Moderate, there's a possibility this will introduce a data race, but even if that's the case, it'll be when the engine is being terminated. Release Notes: Added Co-authored-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

htuch reviewed Dec 28, 2017

View reviewed changes

taiki45 mentioned this pull request Jan 5, 2018

build: introduce VERSION file envoyproxy/data-plane-api#394

Merged

taiki45 added 3 commits January 5, 2018 11:08

Revert "cli: show release version with --version"

0c2ae79

This reverts commit db99351. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Generate version number from d-p-a's VERSION file

df715bc

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

taiki45 force-pushed the show-version2 branch from 0118068 to 3a84f89 Compare January 5, 2018 02:08

htuch reviewed Jan 5, 2018

View reviewed changes

taiki45 added 7 commits January 8, 2018 16:13

Merge remote-tracking branch 'origin/master' into show-version2

d6ac3b9

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Pin to latest compatible data-plane-api

19704cf

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Drop version_consistency_test

26d9125

We don't need this. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Simply use defined macro BUILD_VERSION_NUMBER

d6b7047

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Proper shell syntax

7e6d103

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

More flexible test for version outputs

6c21077

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Forgot to remote the data source

d4526c2

Follow-up of 6c21077. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

htuch approved these changes Jan 8, 2018

View reviewed changes

mattklein123 approved these changes Jan 8, 2018

View reviewed changes

mattklein123 merged commit 48ebd9f into envoyproxy:master Jan 8, 2018

taiki45 deleted the show-version2 branch January 9, 2018 02:31

Conversation

taiki45 commented Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taiki45 commented Jan 5, 2018

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

taiki45 commented Dec 28, 2017 •

edited

Loading

htuch Dec 28, 2017 •

edited

Loading