Add debugHelpersRegistry property

[sbueringer]

Fixes: #2679

Description
Add debugHelpersRegistry property, see: #2679

User facing changes (remove if N/A)
New property debugHelpersRegistry in buildConfig

[codecov]

Codecov Report

Merging #3945 into master will decrease coverage by 0.00%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##           master    #3945      +/-   ##
==========================================
- Coverage   71.72%   71.72%   -0.01%     
==========================================
  Files         324      324              
  Lines       12507    12526      +19     
==========================================
+ Hits         8971     8984      +13     
- Misses       2966     2970       +4     
- Partials      570      572       +2     

Impacted Files	Coverage Δ
pkg/skaffold/deploy/transformations.go	100.00% <ø> (ø)
pkg/skaffold/deploy/kubectl.go	67.10% <25.00%> (-0.68%)	⬇️
pkg/skaffold/debug/debug.go	45.16% <40.00%> (ø)
pkg/skaffold/deploy/kustomize.go	77.05% <40.00%> (-0.66%)	⬇️
pkg/skaffold/config/util.go	70.71% <57.14%> (-0.55%)	⬇️
pkg/skaffold/debug/transform.go	88.77% <100.00%> (ø)
pkg/skaffold/deploy/resource/deployment.go	74.48% <0.00%> (-0.77%)	⬇️
pkg/skaffold/debug/transform_nodejs.go	80.86% <0.00%> (-0.33%)	⬇️
pkg/skaffold/debug/transform_jvm.go	94.62% <0.00%> (-0.06%)	⬇️
pkg/skaffold/event/event.go	90.59% <0.00%> (+0.07%)	⬆️
... and 2 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 4fbc068...21e1890. Read the comment docs.

[sbueringer]

@briandealwis If I understand it correctly, I have to create a new version via hack/new_version.sh first. What should be the name of this new version?

I would assume v2beta2?

INFO[0000] Previous Skaffold version: v2alpha4          
INFO[0000] Current Skaffold version: v2beta1            

[briandealwis]

Thanks @sbueringer for your contribution! I've provided some initial feedback below, but I need some more time to mull over some of the changes you're proposing.

Would you mind motivating why do you find adding the debug helper registry location to the skaffold.yaml more important than specifying it in the global configuration in ~/.skaffold/config? Do you have a need to specify different alternative locations for different projects?

Adding the debugHelperRegistry to the KubectlDeployer and KustomizeDeployer seems out of place, but Skaffold's set up makes it difficult to get this information the debug transforms since we can't pass in the debug transform don't have access to the Skaffold Config object. My thought had been to avoid this issue entirely and instead just pull the information from the global config.

[briandealwis]

Ditto to the KubectlDeployer's debugHelpersRegistry: this seems out of place, and indicates that we have some difficulty communicating information to the debug transforms.

[sbueringer]

Same as in kubectl.go

[sbueringer]

Thanks @sbueringer for your contribution! I've provided some initial feedback below, but I need some more time to mull over some of the changes you're proposing.

Would you mind motivating why do you find adding the debug helper registry location to the skaffold.yaml more important than specifying it in the global configuration in ~/.skaffold/config? Do you have a need to specify different alternative locations for different projects?

Oh that was a misunderstanding. Global config sounds good to me. I'll take a look later and rewrite it if possible.To be honest I haven't used skaffold a lot so I wasn't aware of the global configuration and just thought it's the skaffold.yaml.

[sbueringer]

@briandealwis I moved the property to global config. Most of the other comments (all except the Kubectl/Kustomize struct) should be addressed.

[briandealwis]

This is looking good @sbueringer — thanks! I'm not so thrilled that we're having to exposing debug stuff within the deployers (but that's not you) so I'll talk some more with the rest of the Skaffold team. I have some ideas how we can improve this.

[sbueringer]

This is looking good @sbueringer — thanks! I'm not so thrilled that we're having to exposing debug stuff within the deployers (but that's not you) so I'll talk some more with the rest of the Skaffold team. I have some ideas how we can improve this.

Okay, no problem.

[sbueringer]

@briandealwis any updates? 😃

[DanielSel]

Hi @sbueringer,

what's your opinion on refactoring the GlobalConfig as well? Along the lines of:

kube-context: <...>
default-repo: my.corp/app/repo
insecureRegistries:
- my.corp
imageOverrides:
  debugHelpers: my.corp/debug-helpers
  kanikoImage: my.corp/kaniko/executor
  kanikoInitImage: my.corp/
survey: <...>
<...>

That way, as a corporation you could distribute the skaffold config via config management to developers and have all config for airgapped clusters in one spot. If I missed any image overrides, feel free to add them.

If you're willing to refacor, I would be up for splitting the work

[sbueringer]

@DanielSel I think it makes sense to do this in a separate PR so this PR doesn't get to big :)

But I'm also not sure if/when this PR will get merged in it's current form. So I would wait for feedback from @briandealwis for now.

[briandealwis]

Sorry, I've gotten stuck trying to pass information around too within Skaffold. Will get back to this soon.

[nkubala]

@briandealwis any update here? @sbueringer looks like you'll need a quick rebase.

[tejal29]

LGTM, verified all the reviews comments were addressed.

[tejal29]

@sbueringer can you please fix the tests?

[briandealwis]

Oh jeepers, I'm sorry @sbueringer for letting this dangle. Let's get it in and I'll address these concerns separately.

[sbueringer]

@briandealwis No problem. I'll fix the tests :)