Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

Replace override paths with JSON pointer style paths #991

Merged
merged 6 commits into from
Jun 24, 2019
Merged

Replace override paths with JSON pointer style paths #991

merged 6 commits into from
Jun 24, 2019

Conversation

shashidharatd
Copy link
Contributor

@shashidharatd shashidharatd commented Jun 14, 2019 •
edited
Loading

Please refer to the design doc here

Ref issue: #520

todos:

  • Add tests
  • Update docs and examples

/cc @marun

@k8s-ci-robot k8s-ci-robot requested a review from marun June 14, 2019 13:36
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 14, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shashidharatd

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 14, 2019
@shashidharatd shashidharatd changed the title WIP: Add Json-Patch style overrides Add Json-Patch style overrides Jun 19, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 19, 2019
@shashidharatd shashidharatd changed the title Add Json-Patch style overrides Replace override paths with JSON pointer style paths Jun 19, 2019
marun
marun reviewed Jun 19, 2019
View reviewed changes
test/common/crudtester.go Outdated
@@ -564,8 +564,10 @@ func (c *FederatedTypeCrudTester) waitForResource(client util.ResourceClient, qu

// Validate that the expected override was applied
if len(expectedOverrides) > 0 {
// TODO: use the json patch library to patch with the overrides and then compare it with cluster object.
Copy link
Contributor

Choose a reason for hiding this comment

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

The CI failure suggests that you may need to implement this strategy to support testing of overriding a path that embeds an index. The nested field copy won't work for that case.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 20, 2019
@marun marun mentioned this pull request Jun 20, 2019
Closed
@shashidharatd shashidharatd force-pushed the json-patch-override branch 2 times, most recently from a19213a to c797a29 Compare June 20, 2019 15:01
@shashidharatd
Copy link
Contributor Author

finally the mechanim to test json-patch style override is working. @marun, ptal once again. Thanks !

@marun
Copy link
Contributor

marun commented Jun 20, 2019

Nice work, LGTM!

@shashidharatd
Copy link
Contributor Author

@marun, PTAL. Thanks !

marun
marun reviewed Jun 23, 2019
View reviewed changes
pkg/controller/util/overrides.go
for i, clusterOverride := range clusterOverrides {
path := clusterOverride.Path
if invalidPaths.Has(path) {
return nil, errors.Errorf("override[%d] for cluster %q has an invalid path: %s", i, clusterName, path)
}
if _, ok := overridesMap[clusterName][path]; ok {
if paths.Has(path) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it makes sense to validate here for duplicate paths anymore given that a path could appear more than once for different operations? Selector-based overrides will complicate things still further. Maybe just remove detection of duplicate paths for now?

pkg/kubefedctl/enable/validation.go
@@ -107,6 +107,9 @@ func federatedTypeValidationSchema(templateSchema map[string]v1beta1.JSONSchemaP
Schema: &v1beta1.JSONSchemaProps{
Type: "object",
Properties: map[string]v1beta1.JSONSchemaProps{
"op": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to validate for the specific values that are supported?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider making at least path and value fields required?

Copy link
Contributor

Choose a reason for hiding this comment

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

Path can be required but value is required by replace and add but not remove.

font
font reviewed Jun 24, 2019
View reviewed changes
Copy link
Contributor

@font font left a comment

Choose a reason for hiding this comment

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

Thanks @shashidharatd! There a few outstanding unresolved conversations, but this looks functionally good so I am merging this in anticipation of another release candidate.
/lgtm

pkg/controller/util/overrides.go
Path string `json:"path"`
Value interface{} `json:"value"`
Value interface{} `json:"value,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason to make path required and value optional? Also, it appears to not match the validation schema as none of these fields are required.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming the reason for it being optional is that interface{} is implicitly a pointer. I don't think there is much value in worrying about this struct since it is just a convenience to enable typed use rather than a public interface. I share your concern about the validation schema, though - these fields really should be required.

pkg/kubefedctl/enable/validation.go
@@ -107,6 +107,9 @@ func federatedTypeValidationSchema(templateSchema map[string]v1beta1.JSONSchemaP
Schema: &v1beta1.JSONSchemaProps{
Type: "object",
Properties: map[string]v1beta1.JSONSchemaProps{
"op": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider making at least path and value fields required?

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2019
@k8s-ci-robot k8s-ci-robot merged commit 84e6d3c into kubernetes-retired:master Jun 24, 2019
@font font mentioned this pull request Jun 24, 2019
Closed
@marun marun mentioned this pull request Jun 25, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@font font font left review comments

@marun marun marun left review comments

Assignees

@font font

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shashidharatd @k8s-ci-robot @marun @font