cli: Add --listening-url-file flag

[pydima]

Closes #13327.

[cockroach-teamcity]

This change is 

[tamird]

Thank you for the contribution! Could you add a simple test, as well? Looks good otherwise.

cc @a-robinson because CLI flags.

---

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

pkg/server/server.go, line 867 at r1 (raw file):

		pgURL, err := s.cfg.PGURL(url.User(security.RootUser))
		if err == nil {
			err = ioutil.WriteFile(s.cfg.ListeningURLFile, []byte(fmt.Sprintf("%s\n", pgURL)), 0644)

I'm not sure we want this newline, but I don't feel strongly. I think a simple test will make the choice clearer.

---

Comments from Reviewable

[pydima]

Thanks for your review, I've added a simple test case.

---

Review status: 4 of 7 files reviewed at latest revision, 1 unresolved discussion.

---

pkg/server/server.go, line 867 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I'm not sure we want this newline, but I don't feel strongly. I think a simple test will make the choice clearer.

My intention here was to follow the way how it's done with PID files and it has a new line, but I'm totally OK to change this.

---

Comments from Reviewable

[tamird]

Looking good! Just a few questions.

---

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/server/server_test.go, line 498 at r2 (raw file):

	defer s.Stopper().Stop(context.TODO())
	defer func() {
		os.Remove(filePath)

would you mind checking the error here?

defer func() {
  if err := os.Remove(filePath); err != nil {
    t.Fatal(err)
  }
}()

---

pkg/server/server_test.go, line 501 at r2 (raw file):

	}()

	testutils.SucceedsSoon(t, func() error {

is this retry loop required? I would have expected StartServer to return only after the file was written.

---

Comments from Reviewable

[pydima]

Review status: 6 of 7 files reviewed at latest revision, 2 unresolved discussions.

---

pkg/server/server_test.go, line 498 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

would you mind checking the error here?

defer func() {
  if err := os.Remove(filePath); err != nil {
    t.Fatal(err)
  }
}()

Done.

---

pkg/server/server_test.go, line 501 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

is this retry loop required? I would have expected StartServer to return only after the file was written.

You're right, looked at wrong place in code.

---

Comments from Reviewable

[tamird]

I think this is almost ready to go. We've had some badness on master, so you'll need to rebase, and then I left a minor comment.

Thanks again for your contribution!

---

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

pkg/server/server_test.go, line 491 at r3 (raw file):

	}

	file, err := ioutil.TempFile(cwd, "listening_url_file")

Would you mind using os.TempDir instead of cwd? Also, could you make the file name include t.Name() so it's clear where it came from, in case that ever matters?

---

pkg/server/server_test.go, line 495 at r3 (raw file):

		t.Fatal(err)
	}
	file.Close()

optional: check this error

---

Comments from Reviewable

[pydima]

Review status: 6 of 7 files reviewed at latest revision, 2 unresolved discussions.

---

pkg/server/server_test.go, line 491 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Would you mind using os.TempDir instead of cwd? Also, could you make the file name include t.Name() so it's clear where it came from, in case that ever matters?

Done.

---

pkg/server/server_test.go, line 495 at r3 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

optional: check this error

Done.

---

Comments from Reviewable

[tamird]

Thank you!