Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/contour: unify client creation #2339

Merged
merged 3 commits into from
Mar 16, 2020

Conversation

davecheney
Copy link
Contributor

Updates #403

Before I can add ingress status support to cmd/contour I need to clean
up the serve method. This change unifies the creation of the various
clients to a single method and improves the naming of the various return
values -- they are infomer factories not informers.

Signed-off-by: Dave Cheney [email protected]

@davecheney davecheney added this to the 1.3.0 milestone Mar 16, 2020
Copy link
Member

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

LGTM, nice

cmd/contour/clients.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 16, 2020

Codecov Report

Merging #2339 into master will decrease coverage by 0.11%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2339      +/-   ##
==========================================
- Coverage   77.91%   77.79%   -0.12%     
==========================================
  Files          59       60       +1     
  Lines        5208     5216       +8     
==========================================
  Hits         4058     4058              
- Misses       1062     1070       +8     
  Partials       88       88
Impacted Files Coverage Δ
cmd/contour/contour.go 4.68% <ø> (+0.84%) ⬆️
cmd/contour/serve.go 0% <0%> (ø) ⬆️
internal/k8s/clients.go 0% <0%> (ø)
cmd/contour/certgen.go 0% <0%> (ø) ⬆️

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 bc54145...bf4a8d3. Read the comment docs.

Updates projectcontour#403

Before I can add ingress status support to cmd/contour I need to clean
up the serve method. This change unifies the creation of the various
clients to a single method and improves the naming of the various return
values -- they are infomer _factories_ not informers.

Signed-off-by: Dave Cheney <[email protected]>
}

// ClientSet returns the Kubernetes Core v1 ClientSet.
func (c *Clients) ClientSet() *kubernetes.Clientset {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These getters are unpleasant, I'll try to clean them up in a future PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeh, I'd be inclined to just make the struct members public, but this is fine too.

Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

LGTM

@davecheney davecheney merged commit 8034262 into projectcontour:master Mar 16, 2020
@davecheney davecheney deleted the issue.403 branch March 16, 2020 04:08
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.

3 participants