-
Notifications
You must be signed in to change notification settings - Fork 136
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
add pvc and emptyDir to function_volumes #1666
Conversation
Welcome @zalsader! It looks like this is your first PR to knative/func 🎉 |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1666 +/- ##
===========================================
+ Coverage 49.81% 62.94% +13.12%
===========================================
Files 81 93 +12
Lines 11148 12183 +1035
===========================================
+ Hits 5553 7668 +2115
+ Misses 5111 3818 -1293
- Partials 484 697 +213
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
@zalsader you need to re-generate schema |
I did that. The command for |
One thing is missing: We need to check that the features are enabled. Do you have suggestions on how to do that? |
@zalsader what features do you mean? |
Sorry I should have been more clear. The PVC and emptyDir are behind a feature flag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good
Just one small change: please move the validation logic func validateVolumes
out of pkg/functions
since in order to execute this (much appreciated) more advanced validation requires k8s.io/api/core/v1
and k8s.io/apimachinery/pkg/api/resource
. We are keeping the core package functions
dependency-free, so I think perhaps pkg/k8s
would be the right place for this validator; taking its dependencies with it.
This is a good point, and actually a larger question, because there are other things I would like the functions client library to be able to inspect about the state of the configuration of the cluster, and interact with the user appropriately (or at least generate helpful error messages). For now, I am fine with this PR being as-is, leaning on documentation, and perhaps issuing a warning to the user in the CLI that if they receive error X they need to enable PVC support. I am going to put this on our agenda for this weeks meeting, and probably open a discussion. |
Signed-off-by: Zuhair AlSader <zuhair@koor.tech>
Signed-off-by: Zuhair AlSader <zuhair@koor.tech>
Signed-off-by: Zuhair AlSader <zuhair@koor.tech>
Signed-off-by: Zuhair AlSader <zuhair@koor.tech>
Signed-off-by: Zuhair AlSader <zuhair@koor.tech>
Signed-off-by: Zuhair AlSader <zuhair@koor.tech>
it needs to be regenrated every time. Signed-off-by: Zuhair AlSader <zuhair@koor.tech>
Signed-off-by: Zuhair AlSader <zuhair@koor.tech>
d10643d
to
ad218dc
Compare
When is the meeting, and would I be able to join? Where would be an appropriate place to issue this warning or write that documentation? |
We have a Knative Community Calendar which includes an open Functions Weekly Meeting on Tuesdays from 14:00-14:30 UTC, as well as many other meetings. Until we have a way to query the connected platform to detect things like whether or not PVCs are enabled, I would issue this warning clearly from within the CLI package when a user successfully adds a PVC volume via the CLI; perhaps even with a link to the documentation of how to enable. Perhaps someone in the meeting will have a better suggestion as a stop-gap until we have a more permanent solution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me @zalsader
I'll /lgtm
this after giving you a chance to take a look at that .PHONY declaration, and perhaps add a helpful warning to the user about this feature requiring special features being installed in the cluster; if you can find a good spot to emit that from the cmd
package.
👍🏻
Makefile
Outdated
@@ -203,6 +203,7 @@ $(BIN_WINDOWS): generate/zz_filesystem_generated.go | |||
##@ Schemas | |||
###################### | |||
schema-generate: schema/func_yaml-schema.json ## Generate func.yaml schema | |||
.PHONY: schema/func_yaml-schema.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure that this is not a .PHONY, because it is actually intended to create the file schema/func_yaml-schema.json
. Did you mean to perhaps write .PHONY: schema-generate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make build
depends on $(CODE)
which includes schema/func_yaml-schema.json
. Since the file exists, make
does not attempt to regenerate that file. This caused an issue later in the CI pipeline which runs make schema-generate
.
Since there was a .PHONY: generate/zz_filesystem_generated.go
, I thought it would be ok to do the same for the schema file. What do you think we should do instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @lkingland is correct here. Modifying pkg/functions/function.go
does indeed cause the schema to be regenerated via this make target:
Lines 206 to 207 in d5b4a8d
schema/func_yaml-schema.json: pkg/functions/function.go | |
go run schema/generator/main.go |
The schema-generate
target should have been a no-op in CI unless the schema/func_yaml-schema.json
changed during the build. This has not historically been a problem, so IMO this change is not needed. Otherwise all looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there was a .PHONY: generate/zz_filesystem_generated.go, I thought it would be ok to do the same for the schema file.
I see; thanks for noticing! I believe that .PHONY: generate/zz_filesystem_generated.go
should actually be removed as well, but that's probably an update for another day/issue. The way it is being generated seems to me not entirely optimal, so it's probably not the best example.
Here's how I understand this rule to work:
schema/func_yaml-schema.json: pkg/functions/function.go
go run schema/generator/main.go
pkg/functions/function.go
is the dependency. The rule will only execute if the target file either doesn't exist or is older than the dependency file. In this case, if function.go
is newer than func_yaml-schema.json
(has modifications), then the rule will be executed, generating the file again via go run...
. This is the intended functionality.
I would like to know the problem you saw in CI. There actually might be something over there we need to fix or clean up! Other than this, this PR looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know what the issue is now; I added a commit. schema/func_yaml-schema.json
should also depend on function_*.go
Signed-off-by: Zuhair AlSader <zuhair@koor.tech>
e1ee328
to
e179d8e
Compare
Signed-off-by: Zuhair AlSader <zuhair@koor.tech>
Assuming tests pass, this is good to go. Thanks for your contribution @zalsader! /lgtm |
@zalsader I neglected to notice that @lkingland had requested this change.
Would you mind please handling this as well? Thanks. |
I think it is already done since I removed the extra dependencies. |
The additional dependencies were removed as requested
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lance, zalsader 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 |
Changes
persistentVolumeClaim
andemptyDir
to the allowed function volumespersistentVolumeClaim
andemptyDir
volumespersistentVolumeClaim
andemptyDir
as options in config/kind api-change
Fixes #1647
Release Note
Docs