Repository navigation
Feature/gometalinter - #176
Conversation
|
Build Succeeded 👏 Build Id: c2825c28-7857-4e75-8bd3-56d693c958ae The following development artifacts have been built, and will exist for the next 30 days:
|
markmandel
left a comment
markmandel
left a comment
There was a problem hiding this comment.
👍 some comment/thoughts on changes - but so happy to see this in place!!!!
I can see it already picked up on stuff I particularly missed, just using gometalinter manually.
|
|
||
| # Build a static binary for the gameserver controller | ||
| build-controller-binary: ensure-build-image | ||
| build-controller-binary: ensure-build-image lint |
There was a problem hiding this comment.
I feel like lint should run on make test (here)
wdyt?
There was a problem hiding this comment.
Makes sense, removed in build targets and put it in test one.
|
|
||
| # Lint the go source code. | ||
| lint: ensure-build-image | ||
| docker run --rm $(common_mounts) -w $(mount_path) $(build_tag) bash -c \ |
There was a problem hiding this comment.
Minor nit - add $(DOCKER_RUN_ARGS) to this line.
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| excluded=$(find . -name "*.go" | grep -v vendor | xargs grep autogenerated | cut -d ':' -f 1 | uniq | sed 's#\./##g') |
There was a problem hiding this comment.
This should probably go in build/build-image with the rest of the gen scripts.
Which will mean it will be automatically copied into the build Docker image so it will be easier to call.
May also want to also bump the ForceUpdate value to force the rebuild.
There was a problem hiding this comment.
Done and nice trick for the rebuild
| @@ -0,0 +1,18 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
| # Copyright 2017 Google Inc. All Rights Reserved. | |||
|
|
||
| // main starts the operator for the gameserver CRD | ||
| func main() { | ||
| func main() { // nolint: gocyclo |
There was a problem hiding this comment.
Not for this PR - but we should make a ticket for this, because we should fix this.
| // main starts a UDP server that received 1024 byte sized packets at at time | ||
| // converts the bytes to a string, and logs the output | ||
| func main() { | ||
| func main() { // nolint: gocyclo |
There was a problem hiding this comment.
Same here, we should make a ticket to fix this one too.
| return m | ||
| } | ||
|
|
||
| // StartInformers starts new fake informers |
There was a problem hiding this comment.
👍 it's so good to have all of this picked up
| assert.False(t, sm.hm.healthy) | ||
|
|
||
| s.Ready() | ||
| _ = s.Ready() |
There was a problem hiding this comment.
I feel like these should actually be
err := s.Ready() # change
assert.Nil(t, err) # add this line
assert.True(t, sm.ready)
assert.False(t, sm.shutdown)For all the below. Better to fix the test, since we're here (no idea why I didn't do this in the first place)
There was a problem hiding this comment.
Sorry for this. Totally missed it too.
2c69845 to
a61fdc5
Compare
|
Rebased and squashed |
|
Build Succeeded 👏 Build Id: 067638c6-89dd-4568-a9ef-7e3f21d21cc3 The following development artifacts have been built, and will exist for the next 30 days:
|
|
This LGTM. @enocom do you have anything extra before we approve and merge this? |
|
LGTM |
markmandel
added
area/build-tools
Fixes #163