Skip to content

Few changes to the configmap package#59

Merged
knative-prow-robot merged 3 commits into
knative:masterfrom
dprotaso:configmap-watcher
Sep 12, 2018
Merged

Few changes to the configmap package#59
knative-prow-robot merged 3 commits into
knative:masterfrom
dprotaso:configmap-watcher

Conversation

@dprotaso
Copy link
Copy Markdown
Member

@dprotaso dprotaso commented Sep 4, 2018

  • New* methods return concrete types
  • defaultImpl is now InformedWatcher
  • fixedImpl is now StaticWatcher
  • Included a new ManualWatcher that mimics the InformedWatcher
    in behaviour. ie. allows updates - making this a true 'mock'

@knative-prow-robot knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 4, 2018
@dprotaso
Copy link
Copy Markdown
Member Author

dprotaso commented Sep 4, 2018

/assign @mattmoor

@dprotaso dprotaso force-pushed the configmap-watcher branch 2 times, most recently from 2965c75 to 1fadde9 Compare September 4, 2018 15:12
Copy link
Copy Markdown
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.

/lgtm
/approve
/hold
Thanks for the cleanup. One nit where I think we can document things better, but otherwise this LGTM.

informer corev1informers.ConfigMapInformer
started bool

ManualWatcher
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is this doing for us?

@knative-prow-robot knative-prow-robot added lgtm Indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 8, 2018
@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, 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

@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. labels Sep 8, 2018
@dprotaso
Copy link
Copy Markdown
Member Author

/test pull-knative-pkg-unit-tests

Copy link
Copy Markdown
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.

/lgtm

@dprotaso thanks again for this. Leaving the hold so that you can coordinate merging here with updates to serving and eventing (feel free to remove when you are ready and ping me on slack when those PRs are ready).

@knative-prow-robot knative-prow-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Sep 11, 2018
- New* methods return concrete types
- defaultImpl is now InformedWatcher
- fixedImpl is now StaticWatcher
- Included a new ManualWatcher that mimics the InformedWatcher
  in behaviour. ie. allows updates - making this a true 'mock'
@knative-metrics-robot
Copy link
Copy Markdown

The following is the coverage report on pkg/.
Say /test pull-knative-pkg-go-coverage to run the coverage report again

File Old Coverage New Coverage Delta
configmap/informed_watcher.go Do not exist 92.6%
configmap/manual_watcher.go Do not exist 100.0%
configmap/static_watcher.go Do not exist 90.0%

@dprotaso
Copy link
Copy Markdown
Member Author

Serving - knative/serving#2006
Eventing - knative/eventing#428

Copy link
Copy Markdown
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.

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2018
@mattmoor
Copy link
Copy Markdown
Member

/hold cancel

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2018
@knative-prow-robot knative-prow-robot merged commit 8fc80de into knative:master Sep 12, 2018
@dprotaso dprotaso deleted the configmap-watcher branch September 12, 2018 14:44
chengjingtao pushed a commit to katanomi/knative-pkg that referenced this pull request Oct 22, 2024
Co-authored-by: qingliu <qingliu@alauda.io>
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. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants