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

Convert PortForwarder to Accessor, move to Deployer #6026

Merged
merged 1 commit into from
Jun 18, 2021

Conversation

nkubala
Copy link
Contributor

@nkubala nkubala commented Jun 16, 2021

Related: #5813, #5936

Description
This change creates a new object, Accessor, which is responsible for accessing deployed resources from Skaffold. This object is a conversion of the original PortForward object, and is a superset of it. The Accessor is moved from the Runner to the Deployer, since accessor-behavior is implicitly tied to the underlying Deployer implementation. See #5809 for a more detailed description of why we need to do this.

This change adds one method to the Deployer interface:

type Deployer interface {
  ...

  GetAccessor() access.Accessor
}

The GetAccessor() method is used by the Runner to retrieve the Accessor implementation and actually orchestrate that behavior in the dev loop.

@nkubala nkubala requested review from yuwenma and a team as code owners June 16, 2021 22:53
@nkubala nkubala requested a review from briandealwis June 16, 2021 22:53
@google-cla google-cla bot added the cla: yes label Jun 16, 2021
@codecov
Copy link

codecov bot commented Jun 16, 2021

Codecov Report

Merging #6026 (9104599) into master (e01d089) will decrease coverage by 0.00%.
The diff coverage is 65.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6026      +/-   ##
==========================================
- Coverage   70.75%   70.75%   -0.01%     
==========================================
  Files         464      466       +2     
  Lines       17928    17956      +28     
==========================================
+ Hits        12685    12704      +19     
- Misses       4313     4321       +8     
- Partials      930      931       +1     
Impacted Files Coverage Δ
pkg/skaffold/access/access_mux.go 0.00% <0.00%> (ø)
pkg/skaffold/deploy/deploy_mux.go 65.90% <0.00%> (-4.83%) ⬇️
pkg/skaffold/deploy/label/labeller.go 68.75% <ø> (ø)
...affold/kubernetes/portforward/kubectl_forwarder.go 69.50% <ø> (-0.22%) ⬇️
...kubernetes/portforward/port_forward_integration.go 0.00% <0.00%> (ø)
pkg/skaffold/access/provider.go 50.00% <50.00%> (ø)
pkg/skaffold/deploy/helm/deploy.go 78.68% <50.00%> (-0.24%) ⬇️
pkg/skaffold/deploy/kpt/kpt.go 81.33% <50.00%> (-0.22%) ⬇️
pkg/skaffold/deploy/kubectl/kubectl.go 68.68% <50.00%> (-0.20%) ⬇️
pkg/skaffold/deploy/kustomize/kustomize.go 75.00% <50.00%> (-0.35%) ⬇️
... and 16 more

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 e01d089...9104599. Read the comment docs.

Comment on lines +28 to +31
type Provider interface {
GetKubernetesAccessor(*kubernetes.ImageList) Accessor
GetNoopAccessor() Accessor
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little bit confusing. Are all providers supposed to provide a no-op and kubernetes accessor?

Do we need a provider here?

Copy link
Contributor Author

@nkubala nkubala Jun 17, 2021

Choose a reason for hiding this comment

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

all providers should contain an implementation for each "style" of deployer we support in skaffold - right now there are only Kubernetes deployers (and they all share the same underlying logging/debugging/accessing implementations), but in the future we'll have alternate implementations.

I like this approach because it makes it clear what the different implementations for each component are, and also makes it clear where they can be obtained from (since they rely on dependencies that are injected via closures on the constructors). definitely open to suggestions for alternative approaches here though.

@nkubala nkubala merged commit 46c8933 into GoogleContainerTools:master Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants