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: Adding volumes and volumeMounts support to Feature Store CR. #4983

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

feat: Adding volumes and volumeMounts support to Feature Store CR. #4983

wants to merge 9 commits into from
+5,208 −1,376

Conversation

lokeshrangineni
Copy link
Contributor

@lokeshrangineni lokeshrangineni commented Jan 29, 2025 •
edited
Loading

What this PR does / why we need it:

This PR is adding support to add Volumes and volumeMounts to the FeatureStore custom resource in go operator code.

Which issue(s) this PR fixes:

4994

Misc

@lokeshrangineni lokeshrangineni changed the title Initial draft version to add volumes and volumeMounts options. feat: Initial draft version to add volumes and volumeMounts options. Jan 29, 2025
tchughesiv
tchughesiv requested changes Jan 29, 2025
View reviewed changes
Copy link
Contributor

@tchughesiv tchughesiv left a comment

Choose a reason for hiding this comment

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

some initial thoughts

infra/feast-operator/api/v1alpha1/featurestore_types.go Outdated Show resolved Hide resolved
infra/feast-operator/api/v1alpha1/featurestore_types.go Outdated Show resolved Hide resolved
infra/feast-operator/api/v1alpha1/featurestore_types.go Show resolved Hide resolved
infra/feast-operator/api/v1alpha1/featurestore_types.go Outdated Show resolved Hide resolved
infra/feast-operator/internal/controller/services/services.go Outdated Show resolved Hide resolved
infra/feast-operator/internal/controller/services/services.go Outdated Show resolved Hide resolved
infra/feast-operator/internal/controller/services/services.go Outdated Show resolved Hide resolved
infra/feast-operator/internal/controller/services/services.go Outdated Show resolved Hide resolved
@lokeshrangineni
Adding the Volume and volumeMount with code review comments. and reba…
348ec68 
…sed with the master.

Signed-off-by: lrangine <[email protected]>

Signed-off-by: lrangine <[email protected]>
@lokeshrangineni lokeshrangineni force-pushed the feature/volume-support-new branch from 733241b to 348ec68 Compare January 29, 2025 23:31
@lokeshrangineni
Incorporating code review comments
539dbf6 
Signed-off-by: lrangine <[email protected]>

Signed-off-by: lrangine <[email protected]>
@lokeshrangineni
Adding tests for volume and volumeMounts.
e96cbd4 
Signed-off-by: lrangine <[email protected]>
@lokeshrangineni
fixed the lint errors.
96a4687 
Signed-off-by: lrangine <[email protected]>

Signed-off-by: lrangine <[email protected]>
@lokeshrangineni
fixed the lint errors.
29e3dae 
Signed-off-by: lrangine <[email protected]>

Signed-off-by: lrangine <[email protected]>
@lokeshrangineni
Keeping only the relevant tests related to volume and volumeMounts. r…
1556412 
…est all are not needed for this test.

Signed-off-by: lrangine <[email protected]>
@lokeshrangineni
Removing the unnecessary loggers which are not needed any more.
9853774 
Signed-off-by: lrangine <[email protected]>
@lokeshrangineni
Moved the util method to its own test file because it is used only once.
82360d7 
Signed-off-by: lrangine <[email protected]>
@lokeshrangineni lokeshrangineni marked this pull request as ready for review January 31, 2025 04:13
@lokeshrangineni lokeshrangineni requested a review from a team as a code owner January 31, 2025 04:13
@lokeshrangineni lokeshrangineni changed the title feat: Initial draft version to add volumes and volumeMounts options. feat: Adding volumes and volumeMounts support to Feature Store CR. Jan 31, 2025
tchughesiv
tchughesiv reviewed Jan 31, 2025
View reviewed changes
Copy link
Contributor

@tchughesiv tchughesiv left a comment

Choose a reason for hiding this comment

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

wasn't one of the use-cases for this feature to support data store tls? it might be good to have a sample in config/samples demonstrating what it might look like to use postgres w/ tls using this new feature...

infra/feast-operator/internal/controller/services/services.go Outdated Show resolved Hide resolved
@lokeshrangineni lokeshrangineni marked this pull request as draft January 31, 2025 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchughesiv tchughesiv tchughesiv requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
ok-to-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lokeshrangineni @tchughesiv