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

Prepare to Add functionality to Replacer interface to restrict setting labels on certain kinds. #2060

Merged
merged 8 commits into from
Sep 11, 2019

Conversation

tejal29
Copy link
Contributor

@tejal29 tejal29 commented May 2, 2019

Relates to #1737 and other deploy issues where labels get added to wrong CRDs.

In this PR, i added a Matcher Interface which is used by replacer to replace objects on a manifest key if it is present.

// Matcher is used by Replacer to match object blob to replace based on  a manifest key in the Manifest
type Matcher interface {
	IsMatchKey(key string) bool
	Matches(v interface{}) bool
}

If the manifest object blob contains a match-key and its value is not matched, then we move to the the next Manifest object blob.
In this PR, i have introduced a new interface. This does not change in functionality.
Currently, all replacers namespaceCollector, labelsSetter, imageSaver and imageReplacer use the anyMatcher which match everything.

The next steps for this feature would be

  • Add a KindMatcher which will match key kind with values Pod, Deployment and Services (deployed Resources)
  • Use the KindMatcher for labelSetter

@tejal29 tejal29 requested a review from dgageot May 2, 2019 23:25
@tejal29 tejal29 changed the title Add methods to Replacer interface to restrict setting labels on certain kinds. [Draft] Add methods to Replacer interface to restrict setting labels on certain kinds. May 8, 2019
@tejal29
Copy link
Contributor Author

tejal29 commented May 8, 2019

Hold, until #2074 is merged since this depends on this.

@ryanbrainard
Copy link

@tejal29 Can you explain what the status of this is? I see it was on hold for #2074 and then closed, but it's not clear where it stands now.

@tejal29 tejal29 reopened this Sep 3, 2019
@codecov
Copy link

codecov bot commented Sep 4, 2019

Codecov Report

Merging #2060 into master will increase coverage by 0.15%.
The diff coverage is 95%.

Impacted Files Coverage Δ
pkg/skaffold/deploy/kubectl/labels.go 78.57% <ø> (ø) ⬆️
pkg/skaffold/deploy/kubectl/namespaces.go 82.6% <ø> (ø) ⬆️
pkg/skaffold/deploy/kubectl/images.go 92.18% <0%> (-0.24%) ⬇️
pkg/skaffold/deploy/kubectl/visitor.go 94.11% <100%> (+1.26%) ⬆️
pkg/skaffold/deploy/kubectl/matcher.go 100% <100%> (ø)
pkg/skaffold/initializer/init.go 59.14% <0%> (-2.15%) ⬇️
pkg/skaffold/sync/sync.go 75% <0%> (-0.21%) ⬇️
pkg/skaffold/kubernetes/client.go 40% <0%> (ø) ⬆️
pkg/skaffold/kubernetes/log.go 38.26% <0%> (ø) ⬆️
pkg/skaffold/deploy/status_check.go 63.23% <0%> (ø) ⬆️
... and 10 more

@tejal29 tejal29 changed the title [Draft] Add methods to Replacer interface to restrict setting labels on certain kinds. Prepare to Add functionality to Replacer interface to restrict setting labels on certain kinds. Sep 4, 2019
@tejal29
Copy link
Contributor Author

tejal29 commented Sep 4, 2019

@dgageot Please take a look. This is ready for review.

@dgageot dgageot merged commit 43e326a into GoogleContainerTools:master Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants