Add environment variable option to set postgres ssl mode #2266

ckcd · 2024-03-01T06:30:09Z

What this PR does / why we need it:
In postgres, sslmode may has different value such as disable, prefer and require. But for now in postgres.go#L52 it only allows disable. So we should add an environment variable option to allow users to specify the value of sslmode.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2244

Checklist:

Docs included if any changes are user facing

(Need to update list of Environment Variables for Katib Components on Katib DB Manager. I will create additional pull request to address the change on website.)

ckcd · 2024-03-01T06:33:38Z

@andreyvelich @anencore94 @johnugeorge Please help to review it if you are available, thanks a lot.

tenzen-y

@ckcd Thank you for this contribution!
Generally lgtm

@kubeflow/wg-automl-leads Could you approve CI?

pkg/db/v1beta1/common/const.go

pkg/db/v1beta1/postgres/postgres.go

pkg/db/v1beta1/postgres/postgres_test.go

ckcd · 2024-03-02T16:39:15Z

@tenzen-y Thanks for your reply! I made some changes according to your comments.

tenzen-y

Implementations are lgtm

tenzen-y · 2024-03-02T16:53:42Z

pkg/db/v1beta1/postgres/postgres_test.go

+	dbName = "host=katib-postgres port=5432 user=katib password= dbname=katib sslmode=require"
+	if getDbName() != dbName {
+		t.Errorf("getDbName returns wrong value %v", getDbName())
+	}


cases := map[string]struct{ updateEnvs map[string]string wantName string }{ "All parameters are default": { ... }, "Set KATIB_POSTGRESQL_SSL_MODE": { ... } } for name, tc := range cases { t.Run(name, func(t *testsing.T) { // TEST }) }

Please could you refine these tests?

@tenzen-y Oh I got your point. I made the change based on your suggestion. And add more test cases for each env. Please help to review it. Thanks.

@ckcd Thanks for your great refining! I'd be appreciated for your effort!

tenzen-y · 2024-03-02T16:56:26Z

Also, this guide would be helpful for first time contributor: https://github.com/kubeflow/katib/blob/master/docs/developer-guide.md#go-development

tenzen-y

A few nits.
Otherwise lgtm

tenzen-y · 2024-03-04T09:19:30Z

pkg/db/v1beta1/postgres/postgres_test.go

+		wantName   string
+	}{
+		"All parameters are default": {
+			updateEnvs: map[string]string{},


Suggested change

updateEnvs: map[string]string{},

I think this initialization isn't needed.

Done, remove the initialization.

tenzen-y · 2024-03-04T09:20:36Z

pkg/db/v1beta1/postgres/postgres_test.go

+	dbName = "host=katib-postgres port=5432 user=katib password= dbname=katib sslmode=require"
+	if getDbName() != dbName {
+		t.Errorf("getDbName returns wrong value %v", getDbName())
+	}


@ckcd Thanks for your great refining! I'd be appreciated for your effort!

tenzen-y · 2024-03-04T09:23:42Z

pkg/db/v1beta1/postgres/postgres_test.go

+			if got := getDbName(); got != tc.wantName {
+				t.Errorf("getDbName() = %v, want %v", got, tc.wantName)
+			}


Suggested change

if got := getDbName(); got != tc.wantName {

t.Errorf("getDbName() = %v, want %v", got, tc.wantName)

}

gotName := getDbName()

if diff := cmp.Diff(tc.wantName, gotName); len(diff) != 0 {

t.Errorf("Unexpected DBName (-want,+got):\n%s", diff)

}

To improve visibility, we should use the cmp.Diff.

Sure, update to cmp.Diff.

tenzen-y · 2024-03-04T11:03:34Z

@ckcd Could you rebase this PR since we have fixed the CI errors here: #2267.

ckcd · 2024-03-05T06:28:04Z

@tenzen-y thanks for your great suggestions! I rebase the code and made the changes. Please take a look, thanks.

tenzen-y

I left a comment for a nit.

tenzen-y · 2024-03-05T08:21:12Z

pkg/db/v1beta1/postgres/postgres_test.go

@@ -18,6 +18,7 @@ package postgres

 import (
 	"fmt"
+	"github.com/google/go-cmp/cmp"


Suggested change

"github.com/google/go-cmp/cmp"

Could you move it to the third group? You can verify import orders to perform make lint on your local.

Sure, update based on the lint.

tenzen-y · 2024-03-05T08:25:11Z

@kubeflow/wg-automl-leads Could you approve CI?

Signed-off-by: Kun Chang <[email protected]>

ckcd · 2024-03-05T13:45:38Z

@tenzen-y thanks, update based on the lint.

ckcd · 2024-03-05T13:45:56Z

@kubeflow/wg-automl-leads Could you approve CI?

tenzen-y

Thank you!

/lgtm
/approve
/hold
to wait for approved CI from @kubeflow/wg-automl-leads

google-oss-prow · 2024-03-05T14:39:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ckcd, tenzen-y

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:

~~OWNERS~~ [tenzen-y]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

andreyvelich

Thank you for making this improvement @ckcd!
Please can you submit PR to add this parameter to the Katib docs ?
https://www.kubeflow.org/docs/components/katib/env-variables/#katib-db-manager

tenzen-y · 2024-03-05T19:30:09Z

/hold cancel

ckcd · 2024-03-06T01:26:53Z

@tenzen-y @andreyvelich thanks a lot for your great help! I will submit PR for the docs.

google-oss-prow bot added the size/S label Mar 1, 2024

google-oss-prow bot requested review from andreyvelich, anencore94 and johnugeorge March 1, 2024 06:30

ckcd force-pushed the ck2402_add_ssl_mode_env branch from 9648254 to 3160836 Compare March 1, 2024 06:31

tenzen-y reviewed Mar 2, 2024

View reviewed changes

pkg/db/v1beta1/common/const.go Outdated Show resolved Hide resolved

pkg/db/v1beta1/common/const.go Outdated Show resolved Hide resolved

pkg/db/v1beta1/postgres/postgres.go Outdated Show resolved Hide resolved

pkg/db/v1beta1/postgres/postgres_test.go Outdated Show resolved Hide resolved

ckcd force-pushed the ck2402_add_ssl_mode_env branch from 3160836 to 44783b1 Compare March 2, 2024 16:37

tenzen-y reviewed Mar 2, 2024

View reviewed changes

ckcd force-pushed the ck2402_add_ssl_mode_env branch from 44783b1 to 47eadeb Compare March 4, 2024 08:17

google-oss-prow bot added size/M and removed size/S labels Mar 4, 2024

tenzen-y reviewed Mar 4, 2024

View reviewed changes

tenzen-y mentioned this pull request Mar 4, 2024

Remove MXNet examples #2267

Merged

1 task

ckcd force-pushed the ck2402_add_ssl_mode_env branch from 47eadeb to 7fa4b3b Compare March 5, 2024 06:25

tenzen-y reviewed Mar 5, 2024

View reviewed changes

Add environment variable option to set postgres ssl mode

b04f811

Signed-off-by: Kun Chang <[email protected]>

ckcd force-pushed the ck2402_add_ssl_mode_env branch from 7fa4b3b to b04f811 Compare March 5, 2024 13:44

tenzen-y reviewed Mar 5, 2024

View reviewed changes

google-oss-prow bot added the do-not-merge/hold label Mar 5, 2024

google-oss-prow bot assigned tenzen-y Mar 5, 2024

google-oss-prow bot added the lgtm label Mar 5, 2024

google-oss-prow bot added the approved label Mar 5, 2024

andreyvelich reviewed Mar 5, 2024

View reviewed changes

google-oss-prow bot removed the do-not-merge/hold label Mar 5, 2024

google-oss-prow bot merged commit a2f3fca into kubeflow:master Mar 5, 2024
59 checks passed

This was referenced Mar 6, 2024

katib: Add KATIB_POSTGRESQL_SSL_MODE variable of Katib DB Manager kubeflow/website#3695

Closed

Katib: Add KATIB_POSTGRESQL_SSL_MODE variable of Katib DB Manager kubeflow/website#3696

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add environment variable option to set postgres ssl mode #2266

Add environment variable option to set postgres ssl mode #2266

ckcd commented Mar 1, 2024 •

edited

Loading

ckcd commented Mar 1, 2024

tenzen-y left a comment

ckcd commented Mar 2, 2024

tenzen-y left a comment

tenzen-y Mar 2, 2024

ckcd Mar 4, 2024

tenzen-y Mar 4, 2024

tenzen-y commented Mar 2, 2024

tenzen-y left a comment

tenzen-y Mar 4, 2024

ckcd Mar 5, 2024

tenzen-y Mar 4, 2024

tenzen-y Mar 4, 2024

ckcd Mar 5, 2024

tenzen-y commented Mar 4, 2024

ckcd commented Mar 5, 2024 •

edited

Loading

tenzen-y left a comment

tenzen-y Mar 5, 2024

ckcd Mar 5, 2024

tenzen-y commented Mar 5, 2024

ckcd commented Mar 5, 2024

ckcd commented Mar 5, 2024

tenzen-y left a comment

google-oss-prow bot commented Mar 5, 2024

andreyvelich left a comment

tenzen-y commented Mar 5, 2024

ckcd commented Mar 6, 2024

-			if got := getDbName(); got != tc.wantName {
-				t.Errorf("getDbName() = %v, want %v", got, tc.wantName)
-			}
+			gotName := getDbName()
+			if diff := cmp.Diff(tc.wantName, gotName); len(diff) != 0 {
+				t.Errorf("Unexpected DBName (-want,+got):\n%s", diff)
+			}

Add environment variable option to set postgres ssl mode #2266

Add environment variable option to set postgres ssl mode #2266

Conversation

ckcd commented Mar 1, 2024 • edited Loading

ckcd commented Mar 1, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

ckcd commented Mar 2, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y commented Mar 2, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y commented Mar 4, 2024

ckcd commented Mar 5, 2024 • edited Loading

tenzen-y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y commented Mar 5, 2024

ckcd commented Mar 5, 2024

ckcd commented Mar 5, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Mar 5, 2024

andreyvelich left a comment

Choose a reason for hiding this comment

tenzen-y commented Mar 5, 2024

ckcd commented Mar 6, 2024

ckcd commented Mar 1, 2024 •

edited

Loading

ckcd commented Mar 5, 2024 •

edited

Loading