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

Merged
merged 6 commits into from
Feb 7, 2025
Merged

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

merged 6 commits into from
Feb 7, 2025
+5,187 −1,400

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 Outdated 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 lokeshrangineni force-pushed the feature/volume-support-new branch from 733241b to 348ec68 Compare January 29, 2025 23:31
@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
@tchughesiv
Copy link
Contributor

@lokeshrangineni please rebase

@lokeshrangineni lokeshrangineni force-pushed the feature/volume-support-new branch 2 times, most recently from 82d407b to 9f9263e Compare February 5, 2025 20:56
@lokeshrangineni
Copy link
Contributor Author

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...

I have provided a working sample here

@lokeshrangineni lokeshrangineni marked this pull request as ready for review February 7, 2025 02:44
tchughesiv
tchughesiv requested changes Feb 7, 2025
View reviewed changes
infra/feast-operator/config/samples/v1alpha1_featurestore_postgres_db_volumes_ssl.yaml Outdated Show resolved Hide resolved
infra/feast-operator/config/samples/v1alpha1_featurestore_postgres_db_volumes_ssl.yaml Outdated Show resolved Hide resolved
infra/feast-operator/config/samples/v1alpha1_featurestore_postgres_db_volumes_ssl.yaml Outdated Show resolved Hide resolved
infra/feast-operator/config/samples/v1alpha1_featurestore_postgres_db_volumes_ssl.yaml Outdated Show resolved Hide resolved
infra/feast-operator/config/samples/v1alpha1_featurestore_postgres_db_volumes_ssl.yaml Outdated Show resolved Hide resolved
infra/feast-operator/config/samples/v1alpha1_featurestore_postgres_db_volumes_ssl.yaml Outdated Show resolved Hide resolved
infra/feast-operator/config/samples/v1alpha1_featurestore_postgres_db_volumes_ssl.yaml Outdated Show resolved Hide resolved
infra/feast-operator/config/samples/v1alpha1_featurestore_postgres_db_volumes_ssl.yaml Outdated Show resolved Hide resolved
infra/feast-operator/config/samples/v1alpha1_featurestore_postgres_db_volumes_ssl.yaml Outdated Show resolved Hide resolved
infra/feast-operator/config/samples/v1alpha1_featurestore_postgres_db_volumes_ssl.yaml Outdated Show resolved Hide resolved
@lokeshrangineni
Copy link
Contributor Author

@tchughesiv - Great feedback. I have incorporated all the review comments. PTAL whenever you have some time.

@lokeshrangineni lokeshrangineni force-pushed the feature/volume-support-new branch 2 times, most recently from bc902db to 5ea2b6b Compare February 7, 2025 19:58
@lokeshrangineni lokeshrangineni force-pushed the feature/volume-support-new branch from 5ea2b6b to cd431ab Compare February 7, 2025 20:11
tchughesiv
tchughesiv requested changes Feb 7, 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.

now that you've rebased, we should be able to further simplify the sample CR

infra/feast-operator/config/samples/v1alpha1_featurestore_postgres_db_volumes_ssl.yaml Outdated Show resolved Hide resolved
tchughesiv
tchughesiv requested changes Feb 7, 2025
View reviewed changes
Copy link
Contributor

@tchughesiv tchughesiv left a comment •
edited
Loading

Choose a reason for hiding this comment

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

the whole reason for creating the secret and then specifying...

          envFrom:
            - secretRef:
                name: postgres-secret

is to use the secret key/values as env vars within the container.

infra/feast-operator/config/samples/v1alpha1_featurestore_postgres_db_volumes_ssl.yaml Outdated Show resolved Hide resolved
infra/feast-operator/config/samples/v1alpha1_featurestore_postgres_db_volumes_ssl.yaml Outdated Show resolved Hide resolved
tchughesiv
tchughesiv approved these changes Feb 7, 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.

lgtm

@tchughesiv tchughesiv merged commit ec6f1b7 into feast-dev:master Feb 7, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchughesiv tchughesiv tchughesiv approved these changes

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