so I can type "make" or "make test" (or "make e2e") on a mac and it will make the docker images needed and both the local native binaries and linux docker images without me having to change any env var ? if so 👍 💯 🎉

/lgtm

"make" (which basically implies "make test") apparently depends on whether you've got k8s installed and configured (I thought the go unit tests were supposed to be standalone but clearly aren't)--in my original change I was tempted to cut mixer, pilot, and broker tests from "make test" on Mac but they don't pass on my Linux system either so I backed off on that.

As for docker, it at least won't get hung up at the generate_cert step--I don't have docker installed on my Mac, but if I turn all the "docker build" commands into "echo"s of same then "make docker" and "make GOOS=linux docker" run to completion on my Mac.

I was tempted to cut mixer, pilot, and broker tests from "make test" on Mac

we have some work needed to move tests that aren't unit tests (that require either a real or simulated k8s env) from the real unit tests/self contained/parallelizable ones. but that doesn't mean we shouldn't start trying to have make test pass on a mac, or at least start running some/most tests (afaik mixer tests are split between unit and integration etc)

I don't have docker installed on my Mac

having docker installed isn't an unreasonable requirement, specially when working on istio's build :-)

Looks like the linter on Mac doesn't agree with the one in CircleCI, so I'll have to massage that part a bit more before this can be merged.

As for the failures I saw, here's what I initially saw (my lack of certain tools being installed is somewhat intentional, as it helps to make it more obvious when new dependencies are added):

ok  	istio.io/istio/mixer/pkg/config	0.234s
2018-03-16T20:21:13.694968Z	info	Using user home k8s config - might affect real cluster ! Not found:  /Users/delco/istio/go/src/istio.io/istio/mixer/pkg/config/crd/../../../../pilot/pkg/kube/config
--- FAIL: TestGetAPIServerExtensionCACert (0.00s)
	admit_test.go:489: stat /Users/delco/.kube/config: no such file or directory
FAIL
FAIL	istio.io/istio/mixer/pkg/config/crd	1.264s

ok  	istio.io/istio/broker/pkg/model/osb	0.024s
2018-03-16T20:37:20.283155Z	info	using in-cluster configuration
2018-03-16T20:37:20.283203Z	warn	Neither --kubeconfig nor --master was specified.  Using the inClusterConfig.  This might not work.
2018-03-16T20:37:20.283214Z	warn	error creating inClusterConfig, falling back to default config: unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined
--- FAIL: TestStoreInvariant (0.00s)
	client_test.go:68: invalid configuration: no configuration has been provided
2018-03-16T20:37:20.283737Z	info	using in-cluster configuration
2018-03-16T20:37:20.283805Z	warn	Neither --kubeconfig nor --master was specified.  Using the inClusterConfig.  This might not work.
2018-03-16T20:37:20.283827Z	warn	error creating inClusterConfig, falling back to default config: unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined
--- FAIL: TestBrokerConfig (0.00s)
	client_test.go:68: invalid configuration: no configuration has been provided
FAIL

2018-03-16T22:11:53.207411Z	info	registering for apiVersion config.istio.io/v1alpha2
2018-03-16T22:11:53.207453Z	info	registering CRD "routerules.config.istio.io"
2018-03-16T22:11:53.208734Z	info	Failed to start in-process serverfailed to register custom resources. Post http://localhost:8080/apis/apiextensions.k8s.io/v1beta1/customresourcedefinitions: dial tcp [::1]:8080: getsockopt: connection refused
FAIL	istio.io/istio/pilot/pkg/bootstrap	0.187s

2018-03-16T22:12:48.495247Z	info	AdmissionReview for /, Kind=: / response=&AdmissionResponse{UID:,Allowed:false,Result:&k8s_io_apimachinery_pkg_apis_meta_v1.Status{ListMeta:ListMeta{SelfLink:,ResourceVersion:,Continue:,},Status:Failure,Message:configuration is invalid: empty key,Reason:BadRequest,Details:nil,Code:400,},Patch:nil,PatchType:nil,}
2018-03-16T22:12:48.503449Z	info	Using user home k8s config - might affect real cluster ! Not found:  /Users/delco/istio/go/src/istio.io/istio/pilot/pkg/kube/admit/../config
--- FAIL: TestGetAPIServerExtensionCACert (0.00s)
	admit_test.go:456: kubernetes configuration file "/Users/delco/.kube/config" does not exist
FAIL
FAIL	istio.io/istio/pilot/pkg/kube/admit	0.199s

2018-03-16T22:13:18.795167Z	info	Using user home k8s config - might affect real cluster ! Not found:  /Users/delco/istio/go/src/istio.io/istio/pilot/pkg/serviceregistry/kube/config
--- FAIL: TestServices (0.00s)
	controller_test.go:39: kubernetes configuration file "/Users/delco/.kube/config" does not exist

for linters we have to use an image, afaik, to get uniform results

Codecov Report

Merging #4338 into master will decrease coverage by 1%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #4338    +/-   ##
=======================================
- Coverage      73%     73%   -<1%     
=======================================
  Files         297     297            
  Lines       25324   25194   -130     
=======================================
- Hits        18275   18147   -128     
- Misses       6286    6296    +10     
+ Partials      763     751    -12

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0ddbf5...4d7facc. Read the comment docs.

/lgtm cancel //PR changed after LGTM, removing LGTM. @ldemailly @mattdelco

/lgtm

/lgtm cancel //PR changed after LGTM, removing LGTM. @ldemailly @mattdelco

/approve

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geeknoid, ldemailly

The full list of commands accepted by this bot can be found here.

/test istio-pilot-e2e

AFAIK the plan is for developers to locally run both 'real unit tests' (golden files, no deps) and small tests - requiring an apiserver, but fast.

I don't mind if they are in separate targets - but one way or another you will need an apiserver and etcd.
Louis got the new docker for mac working - which apparently includes an apiserver.

On Linux the tests rely on 'testEnvLocal..' to be run first - not sure how it got removed from the deps of 'test'. AFAIK the tests are passing if this is run first ( circle is calling make test after setting up the local env with same script). I strongly suggest changing that script to point to Louis' docker env on Mac, so
all tests (including those testing k8s registry ) are run.

AFAICT it looks like the execution of 'testEnvLocal...' was explicitly added to the various CIs. I'm not sure if it got removed from the deps of 'test' so that the CIs have more flexibility in which env to start up first (but if so it seems like 'test' should have been kept simple & standalone for devs and CIs could run something else that skips the setup step). Anyway, I tried to make testEnvLocalK8S.s have better support for Mac but I seem to have hit a roadblock that k8s doesn't seem to ship a compiled kube-apiserver for Mac.

/test istio-unit-tests

In Makefile I set up LOCAL_GOARCH and LOCAL_GOOS based on the currently running system, and then use these as defaults for GOARCH and GOOS. The LOCAL_xxx is then passed to gobuild.sh when building in go/bin (to override its tendency to default to Linux/amd64 for unset vars).

shouldn't just be the other way around, default is native, except for docker where it must be linux ?
(ignoring the ibm390 docker images for a second)

apiserver

that part should be optional until we have something turnkey

I guess my point was that you don't need any variable set at all to do the default go behavior and only when making docker images do we need to use linux (and static etc...) builds
it's back to why is there even a gobuild.sh; there should only be a "dockerbuild.sh" or even better build docker images using docker (like https://github.com/istio/fortio/blob/master/Dockerfile#L36)

I think bin/gobuild.sh mostly exists to emulate Bazel's get_workspace_status behavior. Both bin/gobuild.sh and bin/get_workspace_status can be ported to Make (ideally with something like the "BUILDINFO" caching behavior that afaik no build/test system has yet adopted). The default to Linux/amd64 in gobuild.sh might be a reflection of some earlier philosophies I've heard expressed that the Makefiles were mostly for release builds/tests (i.e., Linux), and go devs would just use native tools (though I'm not clear on how go tools would have picked up the get_workspace_status functionality).

When native is desired I only explicitly set GOOS/GOARCH when calling gobuild.sh. For other cases (e.g., "go get" or "go install") these parameters are unset (this is probably more evident in the other PR I just made since it downloads more tools).

i'd strongly prefer to head in the direction of "make" doing the right thing, without having to workaround bad defaults inside gobuild.sh; I feel like a broken record but afaik only Docker images needs to set any env, and would not even if we were building multi-stage builds like we should be.

but we can do baby steps back to sanity I suppose... or 1 big cleanup... up to you

i'd strongly prefer to head in the direction of "make" doing the right thing, without having to workaround
bad defaults inside gobuild.sh; I feel like a broken record but afaik only Docker images needs to set
any env, and would not even if we were building multi-stage builds like we should be.

I'm not clear if "make doing the right thing" means change gobuild.sh to not default to linux/amd64 (and thus the Makefile wouldn't need to override this). If so, then I'd have in mind to do this separately (possibly by getting rid of gobuild.sh). Otherwise, you'll need to be more explicit about what you had in mind. I think with this current change the invocations of make basically fit people's expectations (i.e. make compiles for the native OS unless you opt-in to cross-compiles), even though the internal means of carrying things out are a bit convoluted.

So they do have kubeapiserver builds for mac ? I didn't know this - it simplifies things.

The apiserver isn't there yet, at least not in the expected bucket (I examined the bucket contents to make sure it wasn't just a matter of change in file name/type convention -- from what I saw the Mac bucket only has a few files in it). I did change part of the kube-apiserver URL from "linux" to "${LOCAL_GOOS}" for the sake of completeness and consistency, but that just means that on Mac bin/testEnvLocalK8S.sh will now fail at trying to download a non-existent file, rather than downloading a Linux binary and failing when "kill -0 $(cat $LOG_DIR/apiserver.pid)" can't find a running process. To help avoid confusion I added a "bucket doesn't contain a kube-apiserver for darwin" comment (so someone doesn't think this did work at some point in the past). Perhaps I should've gone a step further and just have the script explicitly fail with msg on Mac, but I wasn't sure if some people were going so far as to compile K8s for their Mac.

I guess it's a good first step if it just works on a mac; we can fix how it ends up working later (ie getting rid of the env vars/ gobuild.sh etc) - whichever you can do - thanks !

@mattdelco PR needs rebase

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geeknoid, ldemailly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

@mattdelco: The following tests failed, say /retest to rerun them all:

After the rebase it looks like another PR opted for the name "GOOS_LOCAL" over "LOCAL_GOOS" so I renamed the vars in this PR to match (and renamed the new "LOCAL_GOARCH" to "GOARCH_LOCAL" to stay consistent, even though it seems odd to me to have some vars called LOCAL_xxx and others called xxx_LOCAL).

I'd request a retest on the 2 failures but it looks like nobody's managed to pass them in the last ~6 hours.