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

Encode artifact image-name and container WORKDIR in container debug info #3564

Merged
merged 1 commit into from
Jan 23, 2020

Conversation

briandealwis
Copy link
Member

Relates to #3122

Should merge before: xxx

Description

debug annotates podspecs with debugging configuration information about its debuggable containers. This debug configuration information was being passed around as map, but it was becoming a bit unwieldy. This PR:

  1. Turns this map into an object (ContainerDebugConfiguration).
  2. Turns the debugging ports into a proper map, instead of just being inlined to the config map.
  3. Extracts and encodes the corresponding artifact's image-name and the container working directory into this new ContainerDebugConfiguration.

One side-effect is that the serialized JSON doesn't sort the same.

The image-name and working-directory is information requested by the IDEs in #3122.

User facing changes

The pod annotation format for the debug configuration information has changed slightly. The IDEs do not currently use this information. Otherwise there should be no visible changes.

Next PRs.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Mentions any output changes.
  • Adds documentation as needed: user docs, YAML reference, CLI reference.
  • Adds integration tests if needed.

Reviewer Notes

  • The code flow looks good.
  • Unit test added.
  • User facing changes look good.

Release Notes

I don't think this change is worth mentioning since there are no known consumers.

`debug` annotates podspecs with debugging configuration information
about its debuggable containers.  This debug configuration information
was being passed around as map, but it was becoming a bit unwieldy.
This PR turns this map into an object (ContainerDebugConfiguration),
and further encodes the corresponding artifact's image-name and the
container working directory.
@codecov
Copy link

codecov bot commented Jan 22, 2020

Codecov Report

Merging #3564 into master will decrease coverage by <.01%.
The diff coverage is 91.66%.

Impacted Files Coverage Δ
pkg/skaffold/debug/debug.go 45.16% <0%> (-1.51%) ⬇️
pkg/skaffold/debug/transform_jvm.go 94.73% <100%> (ø) ⬆️
pkg/skaffold/debug/transform_python.go 85.05% <100%> (ø) ⬆️
pkg/skaffold/debug/transform_nodejs.go 78.64% <100%> (ø) ⬆️
pkg/skaffold/debug/transform_go.go 86.17% <100%> (ø) ⬆️
pkg/skaffold/debug/transform.go 89.37% <100%> (+0.13%) ⬆️

Copy link
Contributor

@dgageot dgageot left a comment

Choose a reason for hiding this comment

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

LGTM with a nit

@@ -114,14 +115,14 @@ func (t testTransformer) RuntimeSupportImage() string {
return ""
}

func (t testTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator) map[string]interface{} {
func (t testTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator) *ContainerDebugConfiguration {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you return a value rather than a pointer?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is a vestige prior to introducing IsApplicable(). I'll do that as a separate PR if that's ok.

@dgageot dgageot self-assigned this Jan 23, 2020
@dgageot dgageot merged commit 4c38a79 into GoogleContainerTools:master Jan 23, 2020
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.

3 participants