Skip to content

report git sha on welcome request#1435

Merged
janl merged 4 commits intomasterfrom
feat/show-git-hash-in-info
Jul 17, 2018
Merged

report git sha on welcome request#1435
janl merged 4 commits intomasterfrom
feat/show-git-hash-in-info

Conversation

@janl
Copy link
Member

@janl janl commented Jul 13, 2018

Closes #1309

@janl janl requested a review from wohali July 13, 2018 12:27
@janl janl added this to the 2.2.0 milestone Jul 13, 2018
@janl janl force-pushed the feat/show-git-hash-in-info branch 2 times, most recently from f292602 to 0a6447f Compare July 13, 2018 14:22
Copy link
Member

@wohali wohali left a comment

Choose a reason for hiding this comment

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

Not 100% sure, but I think this breaks the release process.

Makefile Outdated
endif
endif


Copy link
Member

Choose a reason for hiding this comment

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

unnecessary whitespace

Makefile.win Outdated
SHELL=cmd.exe
REBAR?=$(shell where rebar.cmd)
IN_RELEASE = $(shell if not exist .git echo true)
COUCHDB_VERSION_SUFFIX = -$(shell git rev-parse --short --verify HEAD)
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: why? the way it is now does less work

string:strip(Version0, right)
end,

VersionSuffix = case os:getenv("COUCHDB_VERSION_SUFFIX") of
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this will t work when building from a release tarball, when we're not inside of git. Test case should be:

./configure -c
make dist
cd apache-couchdb-*
./configure -c
make couch

which I expect to fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

You’re right. lemme fix

@janl janl force-pushed the feat/show-git-hash-in-info branch from 0a6447f to c8b0b0a Compare July 13, 2018 15:03
@wohali
Copy link
Member

wohali commented Jul 13, 2018

@janl No, this is still broken. make dist should produce a tarball at anytime that includes the git sha.

Running my test case above on your branch, with no tags, I get this:

$ curl localhost:15984
{"couchdb":"Welcome","version":"2.2.0","git_sha":"","features":["pluggable-storage-engines","scheduler"],"vendor":{"name":"The Apache Software Foundation"}}

The git sha should be 'baked' into the result of make dist regardless of whether we're on a tag or not. This allows us to distinguish actual releases from RCs from merely snapshot builds.

@janl
Copy link
Member Author

janl commented Jul 13, 2018

@wohali can you show all steps preceding that curl?

The way it should™ work is that it gets baked into rebar at make time, but I might be misreading things.

@wohali
Copy link
Member

wohali commented Jul 13, 2018

cd ~/couchdb
git reset --hard && git clean -ffdx
./configure -c
make dist
cd apache-couchdb-2.2.0-c8b0b0a2e
./configure -c
make couch
dev/run -n 1 >/dev/null 2>&1
sleep 10 && curl localhost:15984

@janl
Copy link
Member Author

janl commented Jul 14, 2018

I tried a good while trying to get this going, but it is not going to be a trivial patch.

The problem is as follows: the git sha is only known at make dist time. At that point, make dist runs git archive to build the tarball. That means that only files that have been committed to the source tree make it into the source archive. In order to get the git sha in there, we’d have to make a commit. Or to ament the tarball with a new file that contains the git sha. The former doesn’t sound right to me as that would make the tarball dependent on a commit that will only exist on the release manager’s machine, plus those commits would never make sense to share. The second option might be worth investigating, but I ran out of time for this. Dropping from 2.2.0 unless someone picks this up.

@janl janl removed this from the 2.2.0 milestone Jul 14, 2018
@janl janl force-pushed the feat/show-git-hash-in-info branch from e1bb5e8 to 91fad83 Compare July 15, 2018 17:54
@janl
Copy link
Member Author

janl commented Jul 15, 2018

turns out this was easier after all. After the git archive, we do a bunch of things to the tarball, like copying the docs and fauxton builds into it, so I’m now appending git_sha to version.mk and have Makefile[.win] -> src/couch/rebar.config.script -> couch_server.erl pick it up:

{"couchdb":"Welcome","version":"2.2.0","git_sha":"88e9b5e25","features":["pluggable-storage-engines","scheduler"],"vendor":{"name":"The Apache Software Foundation"}}

@janl janl added this to the 2.2.0 milestone Jul 15, 2018
@janl janl force-pushed the feat/show-git-hash-in-info branch from 756b75b to b12be1a Compare July 15, 2018 18:31
@janl janl force-pushed the feat/show-git-hash-in-info branch from b12be1a to ace1b7f Compare July 15, 2018 18:42
@wohali
Copy link
Member

wohali commented Jul 15, 2018

@janl I will test this tomorrow.

What do you think about dropping the --short everywhere, so we get the full hash?

@janl
Copy link
Member Author

janl commented Jul 15, 2018

@wohali no strong opinion either way. Feel free to amend my patch directly.

One thing I could see people ask is to not make this info available on a non-admin endpoint (I know), what's your thinking on that?

Unfortuantely, #1437 brought in a build bug that caused dist tarballs
to be created always using the last tag that could be found on the
tree. This lead to `master` building tarballs labelled `2.1.0`.

The new approach includes extensive comments to explain the approach,
fixes the bug, and for an encore adds -dirty if you're building a
CouchDB with local changes that aren't committed to git.
@wohali
Copy link
Member

wohali commented Jul 16, 2018

While reviewing this PR, I found that our fixes to improve the 2.1.2 RC process accidentally broke building "snapshot" dist tarballs from master (useful if you want to run bleeding edge releases, for instance). This, in turn, broke our Jenkins CI setup.

I've fixed the bug, and also added dirty detection (which marks a release as -dirty in the tag if it's been locally modified.)

Like the commits prior, this absolutely requires the repo to be inside of git. If we switch version control systems ever, we'll have to redo this part of the Makefile.

Examples

Building this PR

# ./configure && make dist
Done: apache-couchdb-2.2.0-794ea816c.tar.gz
# dev/run -n 1 >/dev/null &
# curl localhost:15984
{"couchdb":"Welcome","version":"2.2.0-794ea816c","git_sha":"794ea816c","features":["pluggable-storage-engines","scheduler"],"vendor":{"name":"The Apache Software Foundation"}}
# When building and running from the generated tarball:
{"couchdb":"Welcome","version":"2.2.0","git_sha":"794ea816c","features":["pluggable-storage-engines","scheduler"],"vendor":{"name":"The Apache Software Foundation"}}

I think that, perhaps, we want to keep the sha suffix on this one inside the tarball. It's not a tagged release of any sort. Then again, the Jenkins auto-built tarballs rarely make their way out of the developer community, and if they do, people use them at their own peril. The presence of the git_sha is all that's necessary to distinguish this from a real release.

This is a minor concern and I don't think blocks this PR.

Tagging this branch (locally!!!) 2.2.0-RC1:

# ./configure -c && make dist
Done: apache-couchdb-2.2.0-RC1.tar.gz
# dev/run -n 1 >/dev/null &
# curl localhost:15984
{"couchdb":"Welcome","version":"2.2.0","git_sha":"794ea816c","features":["pluggable-storage-engines","scheduler"],"vendor":{"name":"The Apache Software Foundation"}}
# When building and running from the generated tarball:
{"couchdb":"Welcome","version":"2.2.0","git_sha":"794ea816c","features":["pluggable-storage-engines","scheduler"],"vendor":{"name":"The Apache Software Foundation"}}

Everything is as expected: the tarball clearly says 2.2.0-RC1, but the build acts like a real 2.2.0 release, from tarball or from the free.

Tagging this branch (locally!!!) 2.2.0 without removing the 2.2.0-RC1 tag (mimicing a real release):

# ./configure -c && make dist
Done: apache-couchdb-2.2.0.tar.gz
# dev/run -n 1 >/dev/null &
# curl localhost:15984
{"couchdb":"Welcome","version":"2.2.0","git_sha":"794ea816c","features":["pluggable-storage-engines","scheduler"],"vendor":{"name":"The Apache Software Foundation"}}
# When building and running from the generated tarball:
{"couchdb":"Welcome","version":"2.2.0","git_sha":"794ea816c","features":["pluggable-storage-engines","scheduler"],"vendor":{"name":"The Apache Software Foundation"}}

Again, everything is as expected. Tarball and both built binaries match.

After making a trivial change to a file (dirty repo) and running the build again:

# ./configure -c && make dist
Done: apache-couchdb-2.2.0-dirty.tar.gz
# dev/run -n 1 >/dev/null &
# curl localhost:15984
{"couchdb":"Welcome","version":"2.2.0-dirty","git_sha":"794ea816c","features":["pluggable-storage-engines","scheduler"],"vendor":{"name":"The Apache Software Foundation"}}
# When building and running from the generated tarball:
{"couchdb":"Welcome","version":"2.2.0","git_sha":"794ea816c","features":["pluggable-storage-engines","scheduler"],"vendor":{"name":"The Apache Software Foundation"}}

This is fine for a first pass. It'd be nice if the asset inside the tarball reflected in the sha that it's been messed with, but that's a new ticket in my mind.

Removing all tags, then tagging this branch (locally!!!) 2.3.4-RC1:

# ./configure -c && make dist
Done: apache-couchdb-2.3.4-RC1.tar.gz
# dev/run -n 1 >/dev/null &
# curl localhost:15984
{"couchdb":"Welcome","version":"2.3.4","git_sha":"794ea816c","features":["pluggable-storage-engines","scheduler"],"vendor":{"name":"The Apache Software Foundation"}}
# When building and running from the generated tarball:
{"couchdb":"Welcome","version":"2.2.0","git_sha":"794ea816c","features":["pluggable-storage-engines","scheduler"],"vendor":{"name":"The Apache Software Foundation"}}

This was an invalid test I did to satisfy my curiosity. The result is unusual, but expected: we still rely on the version.mk checked into git to be accurate inside a tarball. (Removing the need to have version.mk checked into the tree is something we could conceivably do later.)

@janl I thought about it, and no one in our repo is likely to collide hashes, and basically ~everyone uses short git hashes, so let's stick with it as is.

As for the info not being visible on a non-admin endpoint...prior art shows Cloudant exposes their internal build numbers on GET /... Honestly I don't think it's worth worrying about. If people are that worried they can block the entire GET / endpoint because they won't want to expose they're running 2.1.0 still! 😉

I still need to test this on Windows - which, incidentally, doesn't support make dist right now (bash dependency). That's up next.

@wohali
Copy link
Member

wohali commented Jul 16, 2018

And the windows build is broken, because the new hyper app doesn't support it /cc @kocolosk

I'll fix Windows issues in another ticket. For now this is +1

@wohali
Copy link
Member

wohali commented Jul 16, 2018

And the windows build is broken, because the new hyper app doesn't support it /cc @kocolosk

I'll fix Windows issues in another ticket. For now this is +1

@janl janl merged commit 1d69790 into master Jul 17, 2018
@wohali wohali deleted the feat/show-git-hash-in-info branch July 18, 2018 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants