Skip to content
This repository was archived by the owner on Mar 15, 2022. It is now read-only.

issue #82#83

Closed
vigith wants to merge 0 commit into
vmware-archive:masterfrom
vigith:master
Closed

issue #82#83
vigith wants to merge 0 commit into
vmware-archive:masterfrom
vigith:master

Conversation

@vigith

@vigith vigith commented Sep 11, 2019

Copy link
Copy Markdown
Contributor

Umbrella Issue #82

Go 1.12

  • Use go .12 and use modules instead of dep (mostly from Use go modules, upgrade dependencies, add skaffold.dev #80)
  • Use a multistage Dockerfile to build/test the project
  • Updated the versions of:
    • Go: v1.9 -> v1.12.x
    • Alpine Linux: v3.6 -> v3.9.
    • Go dependencies, for example: k8s.io/client_go v5.0.0 -> v12.0.0
  • version to 0.3

Multi Sink Support

  • move controller to a separate method so we can inject fake client for testing
  • ability to pass context to controller and to sinks
  • moving loading viper configs to a different method
  • support concurrency for UpdateEvents by using channels (fan-out)
  • multi sink (Support for Multiple Sinks #79)
  • added new config format support and is backward compatible
  • add tests for sinks which i modified
  • ran go fmt

Signed-off-by: Vigith Maurice vigith@gmail.com

@vigith vigith force-pushed the master branch 2 times, most recently from 623dba1 to 8360e58 Compare September 11, 2019 17:59
@vigith vigith changed the title most of #80 (took Dockerfile, Makefile and go.mod) go1.12 without vendor directories Sep 11, 2019
@vigith vigith changed the title go1.12 without vendor directories #82 Sep 14, 2019
@vigith vigith changed the title #82 issue #82 Sep 14, 2019
@johnSchnake

Copy link
Copy Markdown
Contributor

@vigith This is great, thanks for putting so much effort into this. Would it be possible to split this up into a few smaller PRs though?

I think it is simply too much to review in its current form. If you were to split it up I could tackle one concern at a time:

  • updating go/modules/deps without any functional changes
  • updating docker/build without functional changes
  • multi-sink support

@vigith

vigith commented Sep 23, 2019

Copy link
Copy Markdown
Contributor Author

Surely, I can. I have a question though. I think I will have update the Docker file if I change the golang version because the Dockerfile is pointing to an older go version.

Can I merge the following to 1 single PR?

  • updating go/modules/deps without any functional changes
  • updating docker/build without functional changes

@johnSchnake

Copy link
Copy Markdown
Contributor

I'd rather the PR that updates the go modules/etc just update the part of the Dockerfile that needs it (i.e. update the referred to go version). I just wanted the other dockerfile changes (multistage builds) to be a separate tiny PR for clarity.

@johnSchnake

Copy link
Copy Markdown
Contributor

Also, could you clarify the relationship of this PR with #80 ? I see it also updates go deps and modules but had a very different diff. Were you both just happening to start that work independently or had ya'll coordinated in some way that I need to understand?

@vigith

vigith commented Sep 23, 2019

Copy link
Copy Markdown
Contributor Author

#80 uses mod=vendor, mine removes vendor all together. Otherwise it is the same. Also, vendor brings lots of changes every time we change the code to update go version or when we add a new sink #77 (comment). go 1.13 supports module mirroring, IMHO vendor is not required.

@vigith

vigith commented Sep 26, 2019

Copy link
Copy Markdown
Contributor Author

i have opened #87 which tackles update to go 1.12

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants