Go services: common startup and shutdown message

[lukedirtwalker]

Contributes to #2169

---

This change is 

[klausman]

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker)

---

go/Makefile, line 15 at r1 (raw file):

LOCAL_PKGS = $(patsubst %, ./%/..., $(LOCAL_DIRS))
LOCAL_GOBIN = $(shell realpath -s $$PWD/../bin)
GIT_COMMIT = $(shell git rev-parse --short HEAD)

Note that this will not include any info about uncommited files. I'm not sure there is an easy way to include that information. It would be available with git status -s -uno|wc -l.

---

go/Makefile, line 17 at r1 (raw file):

GIT_COMMIT = $(shell git rev-parse --short HEAD)
GIT_TAG = $(shell git tag -l --points-at HEAD)
BUILD_DATE = $(shell date)

Please use date -Is (ISO8601 date with second-level precision) here.

---

go/lib/env/env.go, line 227 at r1 (raw file):

// RunsInDocker returns whether the current binary is run in a docker container.
func RunsInDocker() bool {
	cmd := exec.Command("bash", "-c", "cut -d: -f 3 /proc/1/cgroup | grep -q '^/docker/'")

This should be /proc/self/cgroup, no?

Also, it would be more portable to just open&read the file with Go functions and look for the string in the returned data.

[lukedirtwalker]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @klausman)

---

go/Makefile, line 15 at r1 (raw file):

Previously, klausman (Tobias Klausmann) wrote…

Note that this will not include any info about uncommited files. I'm not sure there is an easy way to include that information. It would be available with git status -s -uno|wc -l.

Done.

---

go/Makefile, line 16 at r1 (raw file):

LOCAL_GOBIN = $(shell realpath -s $$PWD/../bin)
GIT_COMMIT = $(shell git rev-parse --short HEAD)
GIT_TAG = $(shell git tag -l --points-at HEAD)

We could also use git describe --tags this would print version and commit, what do you think? Example v0.3.1-158-g5ce52e8

---

go/Makefile, line 17 at r1 (raw file):

Previously, klausman (Tobias Klausmann) wrote…

Please use date -Is (ISO8601 date with second-level precision) here.

Done.

---

go/lib/env/env.go, line 227 at r1 (raw file):

Previously, klausman (Tobias Klausmann) wrote…

This should be /proc/self/cgroup, no?

Also, it would be more portable to just open&read the file with Go functions and look for the string in the returned data.

Done.

[klausman]

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)

---

go/Makefile, line 16 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

We could also use git describe --tags this would print version and commit, what do you think? Example v0.3.1-158-g5ce52e8

Yeah, that would be nice, since tags are not immutable.

[klausman]

Reviewed 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

[kormat]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)

a discussion (no related file):
Overall feedback:

• Let's add a --version flag to all shipped binaries (so including things like scmp which don't link against go/lib/env)
• Drop GIT_COMMIT, as GIT_TAG is sufficient.
• Only include the date if this is a release build on CI

---

[kormat]

Reviewed 5 of 8 files at r1, 2 of 3 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker)

---

go/lib/env/logging.go, line 106 at r3 (raw file):

	inDocker, err := RunsInDocker()
	if err != nil {
		log.Error("Unable to determine if running in docker", "err", err)

Hmm. I'm not sure about continuing if this fails. I can't imagine why it would fail, but it seems important enough that we know it happens.

[kormat]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker)

---

go/Makefile, line 16 at r3 (raw file):

LOCAL_GOBIN = $(shell realpath -s $$PWD/../bin)
GIT_COMMIT = $(shell git rev-parse --short HEAD)-$(shell git diff --quiet || echo 'dirty')
GIT_TAG = $(shell git describe --tags)-$(shell git diff --quiet || echo 'dirty')

The - in between should be moved into the echo. If the tree is not dirty, we don't want the output to have a trailing -.

---

go/Makefile, line 17 at r3 (raw file):

GIT_COMMIT = $(shell git rev-parse --short HEAD)-$(shell git diff --quiet || echo 'dirty')
GIT_TAG = $(shell git describe --tags)-$(shell git diff --quiet || echo 'dirty')
BUILD_DATE = $(shell date -Is)

date -u -Is - we want UTC time.

[lukedirtwalker]

Reviewable status: 2 of 16 files reviewed, 4 unresolved discussions (waiting on @klausman and @kormat)

a discussion (no related file):

Previously, kormat (Stephen Shirley) wrote…

Overall feedback:

• Let's add a --version flag to all shipped binaries (so including things like scmp which don't link against go/lib/env)
• Drop GIT_COMMIT, as GIT_TAG is sufficient.
• Only include the date if this is a release build on CI

Done.

• --version added
• GIT_TAG only
• only include the date if we are on a clean tag (as that apparently how CI realizes it has to do a release).

---

---

go/Makefile, line 16 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

The - in between should be moved into the echo. If the tree is not dirty, we don't want the output to have a trailing -.

Done.

---

go/Makefile, line 17 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

date -u -Is - we want UTC time.

Done.

---

go/lib/env/logging.go, line 106 at r3 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Hmm. I'm not sure about continuing if this fails. I can't imagine why it would fail, but it seems important enough that we know it happens.

Done.

[kormat]

Reviewed 1 of 14 files at r4.
Reviewable status: 3 of 16 files reviewed, 7 unresolved discussions (waiting on @klausman, @kormat, and @lukedirtwalker)

---

go/Makefile, line 19 at r4 (raw file):

GIT_TAG = $(shell git describe --tags)$(shell git diff --quiet || echo '-dirty')
LINK_FLAGS =\
	-X \"github.com/scionproto/scion/go/lib/env.StartupBuildChain=$(BUILD_CHAIN)\"

This can be folded into the previous line.

---

go/Makefile, line 21 at r4 (raw file):

	-X \"github.com/scionproto/scion/go/lib/env.StartupBuildChain=$(BUILD_CHAIN)\"
ifeq ($(HEAD_TAG), $(GIT_TAG))
	BUILD_DATE = $(shell date -u -Is)

No need for tabs outside of make target rules.

---

go/Makefile, line 22 at r4 (raw file):

ifeq ($(HEAD_TAG), $(GIT_TAG))
	BUILD_DATE = $(shell date -u -Is)
 	LINK_FLAGS += -X \"github.com/scionproto/scion/go/lib/env.StartupBuildDate=$(BUILD_DATE)\"

Space and tab - trying to cover all possibilities? :)

---

go/Makefile, line 23 at r4 (raw file):

	BUILD_DATE = $(shell date -u -Is)
 	LINK_FLAGS += -X \"github.com/scionproto/scion/go/lib/env.StartupBuildDate=$(BUILD_DATE)\"
	# add commit, because on a clean tag `git describe --tags` doesn't contain the commit.

Is that necessary, though? The tag should only ever apply to a single commit.

---

go/Makefile, line 24 at r4 (raw file):

 	LINK_FLAGS += -X \"github.com/scionproto/scion/go/lib/env.StartupBuildDate=$(BUILD_DATE)\"
	# add commit, because on a clean tag `git describe --tags` doesn't contain the commit.
	GIT_TAG += $(shell git rev-parse --short HEAD)

This is missing a - between the existing tag and the commit id.

[kormat]

Reviewed 6 of 14 files at r4.
Reviewable status: 9 of 16 files reviewed, 7 unresolved discussions (waiting on @klausman, @kormat, and @lukedirtwalker)

[kormat]

Reviewed 7 of 14 files at r4.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @kormat and @lukedirtwalker)

---

go/lib/env/logging.go, line 115 at r4 (raw file):

		fmt.Sprintf("In docker:     %v", inDocker),
		fmt.Sprintf("euid/egid:     %d %d", os.Geteuid(), os.Getegid()),
		fmt.Sprintf("cmd line:      %v", os.Args),

Use %q for this - it'll indicate the grouping of arguments.

[lukedirtwalker]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kormat)

---

go/Makefile, line 19 at r4 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This can be folded into the previous line.

Done.

---

go/Makefile, line 21 at r4 (raw file):

Previously, kormat (Stephen Shirley) wrote…

No need for tabs outside of make target rules.

Done.

---

go/Makefile, line 22 at r4 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Space and tab - trying to cover all possibilities? :)

Done.

---

go/Makefile, line 23 at r4 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Is that necessary, though? The tag should only ever apply to a single commit.

I'd say so, A tag should only ever apply to a single commit, yes but it can in theory be moved.

---

go/Makefile, line 24 at r4 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This is missing a - between the existing tag and the commit id.

Done.

---

go/lib/env/logging.go, line 115 at r4 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Use %q for this - it'll indicate the grouping of arguments.

Done.

[kormat]

Reviewed 6 of 6 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved