Skip to content

Conversation

rychipman
Copy link
Contributor

This PR contains some platform renaming that is better to do in a separate PR from the rest of 2442 for ease of testing. Also wrapped in some refactors/rationalizations of the helper functions and types we're using in the release/ dir.

Only requiring @pmeredit from the get-go, since it's mostly just internal refactors of code he and I have added in the release/ dir.

@rychipman rychipman requested a review from pmeredit February 18, 2020 23:37
@@ -587,7 +584,7 @@ functions:
aws_key: ${aws_key}
aws_secret: ${aws_secret}
local_files_include_filter:
- src/github.com/mongodb/mongo-tools/mongodb-cli-tools-*
- src/github.com/mongodb/mongo-tools/release.*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the same artifact may get uploaded as multiple different names (for example, as a "stable" and a "latest stable" release), it is easier and less confusing to just use release.tgz (and release.msi, etc) here.

@@ -1416,13 +1430,12 @@ buildvariants:
# Amazon x86_64 Buildvariants #
#######################################

- name: amazonlinux64
- name: amazon
Copy link
Contributor Author

Choose a reason for hiding this comment

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

did some work to match the server's platform naming here. also removed the largely-redundant _platform expansion

@@ -0,0 +1,34 @@
package env
Copy link
Contributor Author

Choose a reason for hiding this comment

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

consolidated a bunch of funcs that access env variables into one package

@@ -12,180 +22,185 @@ import (
type Platform struct {
Name string
Arch string
OS string
Pkg string
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as much as possible, trying to move data about platforms into the structs themselves instead of having it in helper funcs that switch on the name/arch fields

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should stick DebArch in here too, but I think that's probably overkill, since not every platform needs that.

@@ -75,13 +78,18 @@ func run(name string, args ...string) (string, error) {
}

func getVersion() string {
desc, err := run("git", "describe")
desc, err := run("git", "describe", "--dirty")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

differentiate between a build run with a clean git status and one run with uncommitted changes


md5sums := make(map[string]string)
// We use the order just to make sure the md5sums are always in the same order.
// This probably doesn't matter, but it looks nicer for anyone inspecting the md5sums file.
md5sumsOrder := make([]string, 0, len(binaries) + len(staticFiles))
md5sumsOrder := make([]string, 0, len(binaries)+len(staticFiles))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

gofmt did this

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably set gofmt to run automatically at some point. But I've always felt that way :D

@@ -83,7 +83,7 @@ set_goenv() {
}

print_ldflags() {
VersionStr="$(git describe)"
VersionStr="$(go run release/release.go get-version)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sure --version is getting its version info from the same place as our packaging code

@@ -1472,7 +1482,7 @@ buildvariants:
# macOS Buildvariant #
#######################################

- name: macOS-1014
- name: macos
Copy link
Contributor

Choose a reason for hiding this comment

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

much better, macos should always be latest.

return true, nil
func (p Platform) DebianArch() string {
if p.Pkg != PkgDeb {
panic("called DebianArch on non-debian platform")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

Copy link
Contributor

@pmeredit pmeredit left a comment

Choose a reason for hiding this comment

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

lgtm!

@rychipman rychipman merged commit ac2734d into mongodb:master Feb 24, 2020
@rychipman rychipman deleted the TOOLS-2442 branch February 24, 2020 22:01
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