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.

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

feat: Feast Operator example with Postgres in TLS mode. #5028

Conversation

lokeshrangineni
Copy link
Contributor

@lokeshrangineni lokeshrangineni commented Feb 7, 2025

Feast Operator example with Postgres in TLS mode.

@lokeshrangineni lokeshrangineni changed the title rough working instructions to set up postgres in TLS mode and setting… feat: Feast Operator example with Postgres in TLS mode. Feb 7, 2025
@lokeshrangineni lokeshrangineni force-pushed the feature/operator-postgres-ssl-example branch from e89ee80 to 4b75e78 Compare February 17, 2025 05:09
@lokeshrangineni lokeshrangineni marked this pull request as ready for review February 17, 2025 05:12
@lokeshrangineni lokeshrangineni requested a review from a team as a code owner February 17, 2025 05:12
@redhatHameed
Copy link
Contributor

@tchughesiv @lokeshrangineni can use same directory operator-quickstart and can add new notebook example with Postgres in TLS mode with that all example related with operator will be same place. WDYT ?

@lokeshrangineni
Copy link
Contributor Author

lokeshrangineni commented Feb 17, 2025

@tchughesiv @lokeshrangineni can use same directory operator-quickstart and can add new notebook example with Postgres in TLS mode with that all example related with operator will be same place. WDYT ?

This example is having multiple notebooks isolates the group of steps. It appears to be the same notebooks as operator-quickstart based on the names but the content is different. It is going to be a further different after I add more description about volumeMounts and FEAST_CA_CERT_FILE_PATH. The abstraction i can do more with the demo notebook which is pretty close in both examples. The options i see are 1) combining them into one makes a big notebook with sub sections. 2) may be is it ok to add sub folders under operator-quickstart one with existing and another with postgres with tls.

any further preferences?

@redhatHameed
Copy link
Contributor

@tchughesiv @lokeshrangineni can use same directory operator-quickstart and can add new notebook example with Postgres in TLS mode with that all example related with operator will be same place. WDYT ?

This example is having multiple notebooks isolates the group of steps. It appears to be the same notebooks as operator-quickstart based on the names but the content is different. It is going to be a further different after I add more description about volumeMounts and FEAST_CA_CERT_FILE_PATH. The abstraction i can do more with the demo notebook which is pretty close in both examples. The options i see are 1) combining them into one makes a big notebook with sub sections. 2) may be is it ok to add sub folders under operator-quickstart one with existing and another with postgres with tls.

any further preferences?

if it's not fitting with existing example, then sure you can go with existing approach.

The idea is to create a note for an example like 03-TLS, then patch the CR to update the TLS configuration, and proceed further to verify that TLS is set up correctly and proceed with existing example accordingly. You will definitely need to configure PostgreSQL with TLS installation for that just delete the existing installation and setup it with TLS.

@tchughesiv
Copy link
Contributor

imo, the cleanest way would be to piggy-back off of the existing quickstart. Just add another .ipynb file, make it #3. And then continue what's already been done with the demo from #2. You could certainly create a subdir for the ssl bits.
https://github.com/feast-dev/feast/tree/master/examples/operator-quickstart

…orporating code review comments.

Signed-off-by: lrangine <[email protected]>
@tchughesiv
Copy link
Contributor

tchughesiv commented Feb 17, 2025

@lokeshrangineni that said, if its a huge ordeal to combine them... i'm completely fine keeping them separate as well

After removing the demo notebook there is not lot of synergy between operator-quickstart. installing postgres and uninstall jupyter notebook are totally different. Installing feast notebook is emphasizing more on the volume support and mounting secrets with different choices. so prefer to keep them separate as we discussed.

… Also fixed some broken links.

Signed-off-by: lrangine <[email protected]>
Signed-off-by: lrangine <[email protected]>
Signed-off-by: lrangine <[email protected]>
Signed-off-by: lrangine <[email protected]>
Signed-off-by: lrangine <[email protected]>
1. Now the numbers are automatically ordered when we render to avoid renumbering when we add an example in between.
2. Added separate section for the feast go operator examples.

Signed-off-by: lrangine <[email protected]>
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.

@lokeshrangineni to ensure this demo continues to work, you may want to specify a version of the bitnami/postgresql helm chart during install. future changes/release could derail this example.

@lokeshrangineni
Copy link
Contributor Author

@lokeshrangineni to ensure this demo continues to work, you may want to specify a version of the bitnami/postgresql helm chart during install. future changes/release could derail this example.

I have incorporated in the commit

@redhatHameed
Copy link
Contributor

@lokeshrangineni WDYT Squashing commits to single ?

@tchughesiv
Copy link
Contributor

@redhatHameed @lokeshrangineni no need to squash them... the merge will

@redhatHameed redhatHameed enabled auto-merge (squash) February 25, 2025 16:27
@lokeshrangineni lokeshrangineni dismissed tchughesiv’s stale review February 25, 2025 17:50

already incorporated or responded to the request.

@redhatHameed redhatHameed merged commit 2c46f6a into feast-dev:master Feb 25, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants