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.

Sign up for GitHub

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

Jump to bottom

feat(templates): remove liveness and readiness from defaults #1555

Merged
merged 2 commits into from
Feb 16, 2023
Merged

feat(templates): remove liveness and readiness from defaults #1555

merged 2 commits into from
Feb 16, 2023

Conversation

lance
Copy link
Member

@lance lance commented Feb 14, 2023

Changes

The liveness and readiness URLs are defined in code as defaulting to /health/liveness and /health/readiness respectively. The deploy command handles these defaults without requiring them to exist in the func.yaml file. Existing tests for defaults and overrides already exist, so this change does not require additional tests to be included.

/kind enhancement

Fixes: #1281

Release Note

removes default endpoints from func.yaml to improve file legibility

@knative-prow knative-prow bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 14, 2023
@lance lance added this to the Release 1.10 milestone Feb 14, 2023
@codecov
Copy link

codecov bot commented Feb 14, 2023 •
edited

Codecov Report

Base: 60.28% // Head: 50.85% // Decreases project coverage by -9.43% ⚠️

Coverage data is based on head (87cf62c) compared to base (5a4803b).
Patch has no changes to coverable lines.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1555      +/-   ##
==========================================
- Coverage   60.28%   50.85%   -9.43%     
==========================================
  Files          75       73       -2     
  Lines       10214    10079     -135     
==========================================
- Hits         6157     5126    -1031     
- Misses       3489     4519    +1030     
+ Partials      568      434     -134
Flag Coverage Δ
integration-tests ?
unit-tests ?
unit-tests-ubuntu-latest 50.85% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/functions/repository.go 72.60% <ø> (-0.36%) ⬇️
pkg/k8s/security_context.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/knative/logs.go 0.00% <0.00%> (-80.42%) ⬇️
pkg/k8s/dialer.go 0.00% <0.00%> (-73.44%) ⬇️
pkg/k8s/persistent_volumes.go 0.00% <0.00%> (-72.40%) ⬇️
pkg/k8s/logs.go 0.00% <0.00%> (-70.00%) ⬇️
pkg/knative/describer.go 0.00% <0.00%> (-66.18%) ⬇️
pkg/knative/lister.go 0.00% <0.00%> (-64.92%) ⬇️
pkg/knative/deployer.go 6.16% <0.00%> (-57.35%) ⬇️
pkg/knative/client.go 0.00% <0.00%> (-53.85%) ⬇️
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lance
Copy link
Member Author

lance commented Feb 14, 2023

/hold
It seems this is a little naive.

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 14, 2023
@lance
Copy link
Member Author

lance commented Feb 14, 2023

/unhold

@knative-prow knative-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 14, 2023
lkingland
lkingland approved these changes Feb 15, 2023
View reviewed changes
Copy link
Member

@lkingland lkingland left a comment

Choose a reason for hiding this comment

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

If it passes tests, this is precisely what I was hoping would work!

/lgtm
/hold for zbynek's massive refactor to the repo to land first, which was submitted one hour prior

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2023
@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 15, 2023
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2023
@lance
feat(templates): remove liveness and readiness from defaults
a5dc522 
The liveness and readiness URLs are defined in code as defaulting to
`/health/liveness` and `/health/readiness` respectively. The deploy
command handles these defaults without requiring them to exist in the
`func.yaml` file. Existing tests for defaults and overrides already
exist, so this change does not require additional tests to be included.

Signed-off-by: Lance Ball <lball@redhat.com>
@lance
fixup: remove default endpoints from repository.go
87cf62c 
Signed-off-by: Lance Ball <lball@redhat.com>
@knative-prow knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 15, 2023
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2023
@lance
Copy link
Member Author

lance commented Feb 15, 2023

@lkingland after rebasing, I need another /lgtm please

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2023
@lkingland lkingland removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 16, 2023
zroubalik
zroubalik approved these changes Feb 16, 2023
View reviewed changes
Copy link
Contributor

@zroubalik zroubalik 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
Copy link

knative-prow bot commented Feb 16, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance, lkingland, zroubalik

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:
  • OWNERS [lance,lkingland,zroubalik]

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

@zroubalik
Copy link
Contributor

/override codecov/project

@knative-prow
Copy link

knative-prow bot commented Feb 16, 2023

@zroubalik: Overrode contexts on behalf of zroubalik: codecov/project

In response to this:

/override codecov/project

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.

@knative-prow knative-prow bot merged commit ab194d4 into knative:main Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zroubalik zroubalik zroubalik approved these changes

@lkingland lkingland lkingland approved these changes

@maximilien maximilien Awaiting requested review from maximilien

@vyasgun vyasgun Awaiting requested review from vyasgun

Assignees

@zroubalik zroubalik

@lkingland lkingland

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/enhancement lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
Functions WG Roadmap
Archived in project
Milestone
Release 1.10
Development

Successfully merging this pull request may close these issues.

Default Health Endpoints
4 participants
@lance @zroubalik @lkingland @knative-prow-robot