-
Notifications
You must be signed in to change notification settings - Fork 0
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
Introduces new "env_var" and "file" fields to Secret to allow specifying name/mountPath on injection #minor #1
Conversation
…injection if exists
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.
Thank you for your contribution!
I see the original issue is marked as closed as it suggests using PodTemplate. Is that not enough? or not ergonomic enough?
protos/flyteidl/core/security.proto
Outdated
|
||
// The name of the environment variable, if the Secret is injected as environment variable. If ommitted, the default | ||
// FLYTE_SECRETS_ENV_PREFIX prefix will be used. | ||
// +optional | ||
string env_name = 5; |
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 only applies to ENV_VAR
mount type. I would make the change as following:
- Mark
MountType mount_requirement = 4 [deprecated=true];
as deprecated. - Add:
oneof MountTarget mount_target { MountEnvVar env_var = 5; MountFile file = 6; } message MountEnvVar { string name = 1; } message MountFile { string path = 1; }
- Update secrets webhook to look at the new field first, if it's nil, it should fallback to the old field.
- Update flytekit to always set both fields;
mount_requirement
andmount_target
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.
Thanks for the feedback! I'll try to add it. Could you explain what you mean with 4? Not sure I get what you mean here.
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.
Added the changes with a working initial version, I'll get to adding a number of tests as well.
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.
Tests & documentation added, let me know what you think :)
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've opened PRs here for the change, I'll close this one.
- Introduces new "env_var" and "file" fields to Secret to allow specifying name/mountPath on injection flyteorg/flytekit#1726
- Introduces new "env_var" and "file" fields to Secret to allow specifying name/mountPath on injection #minor flyteorg/flytepropeller#589
- Introduces new "env_var" and "file" fields to Secret to allow specifying name/mountPath on injection flyteorg/flyteidl#423
- Adds documentation on usage for new Secret fields env_var and file flyteorg/flytesnacks#1017
TL;DR
Introduces new fields to the
Secret
object:env_var
file
Allowing users to directly specify a name or mountPath for the Secret, without having to specify a full PodTemplate(name). The old
mount_requirement
can still be used. Example:Type
Are all requirements met?
Complete description
How did you fix the bug, make the feature etc. Link to any design docs etc
Tracking Issue
Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue
fixes https://github.com/flyteorg/flyte/issues/
Follow-up issue
NA
OR
https://github.com/flyteorg/flyte/issues/