Skip to content

Use configmap parser library from pkg.#7935

Merged
knative-prow-robot merged 1 commit intoknative:masterfrom
markusthoemmes:use-configmap-parser
May 16, 2020
Merged

Use configmap parser library from pkg.#7935
knative-prow-robot merged 1 commit intoknative:masterfrom
markusthoemmes:use-configmap-parser

Conversation

@markusthoemmes
Copy link
Contributor

Proposed Changes

As the title says, uses some shared code moved into pkg here knative/pkg#1329.

Release Note

NONE

@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 13, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 13, 2020
@knative-prow-robot knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/API API objects and controllers area/autoscale labels May 13, 2020
Copy link
Contributor

@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

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

@markusthoemmes: 0 warnings.

Details

In response to this:

Proposed Changes

As the title says, uses some shared code moved into pkg here knative/pkg#1329.

Release Note

NONE

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@markusthoemmes markusthoemmes force-pushed the use-configmap-parser branch from 34b5264 to 1ab48ad Compare May 15, 2020 07:39
@markusthoemmes markusthoemmes changed the title [WIP] Use configmap parser library from pkg. Use configmap parser library from pkg. May 15, 2020
@knative-prow-robot knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/networking and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 15, 2020
@markusthoemmes markusthoemmes force-pushed the use-configmap-parser branch from 1ab48ad to 1e464a6 Compare May 15, 2020 07:40
defaultTagTemplate = template.Must(template.New("tag-template").Parse(DefaultTagTemplate))
templateCache *lru.Cache

// Verify the default template are valid templates.
Copy link
Contributor

Choose a reason for hiding this comment

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

Uber-nit:

Suggested change
// Verify the default template are valid templates.
// Verify the default templates are valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"knative.dev/pkg/apis"
cm "knative.dev/pkg/configmap"
Copy link
Contributor

Choose a reason for hiding this comment

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

not wild about the import rename here, but I see why we're doing it so 🤷.

@julz
Copy link
Contributor

julz commented May 15, 2020

apart from the smallest nit ever,
/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 15, 2020
@vagababov
Copy link
Contributor

/lgtm
/retest

@markusthoemmes
Copy link
Contributor Author

/retest

@markusthoemmes
Copy link
Contributor Author

/test pull-knative-serving-integration-tests

@markusthoemmes
Copy link
Contributor Author

/test pull-knative-serving-integration-tests

Yo prow, retry this!

@markusthoemmes markusthoemmes force-pushed the use-configmap-parser branch from 46ea9fb to 6da8a3a Compare May 15, 2020 17:05
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label May 15, 2020
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Nice!

/lgtm
/approve

@knative-prow-robot knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 15, 2020
@markusthoemmes markusthoemmes force-pushed the use-configmap-parser branch from 6da8a3a to 10f0f78 Compare May 15, 2020 23:15
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 15, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markusthoemmes, mattmoor

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

The pull request process is described here

Details 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

@vagababov
Copy link
Contributor

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 15, 2020
@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/config/defaults.go 97.1% 96.6% -0.6
pkg/network/network.go 82.9% 80.6% -2.4

@knative-test-reporter-robot

The following jobs failed:

Test name Triggers Retries
pull-knative-serving-integration-tests pull-knative-serving-integration-tests
pull-knative-serving-integration-tests
pull-knative-serving-integration-tests
3/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-integration-tests

@knative-prow-robot knative-prow-robot merged commit ea8cf5a into knative:master May 16, 2020
@julz julz mentioned this pull request Jul 23, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers area/autoscale area/networking cla: yes Indicates the PR's author has signed the 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

Development

Successfully merging this pull request may close these issues.

8 participants