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

Add param to MySQL executor #7

Merged
merged 1 commit into from
Jun 9, 2020
Merged

Add param to MySQL executor #7

merged 1 commit into from
Jun 9, 2020

Conversation

mchompalova
Copy link
Contributor

No description provided.

@mchompalova mchompalova requested review from a team May 28, 2020 06:50
gkazakov111
gkazakov111 previously approved these changes May 28, 2020
View reviewed changes
@mchompalova mchompalova force-pushed the fix/add-test-rds-dsn-prefix branch from 4a89cda to 8cd8e15 Compare May 28, 2020 07:44
@mchompalova mchompalova requested a review from a team May 28, 2020 07:51
gkazakov111
gkazakov111 previously approved these changes May 28, 2020
View reviewed changes
@mchompalova mchompalova requested a review from a team May 28, 2020 07:56
karsov
karsov suggested changes May 28, 2020
View reviewed changes
Copy link

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

"TEST_RDS_DSN_PREFIX" and "DB_TEST_URL" have the same meaning. So let's fix this. I suggest the following steps:

  1. In this PR - let's rename "TEST_RDS_DSN_PREFIX" to "TEST_DB_DSN" to be more generic and to reflect the real meaning of its value.
  2. Fix "concept-events-notifications" to use this new variable
  3. Fix "generic-rw-aurora" to use this new variable
  4. Remove "DB_TEST_URL" from the orb.

@mchompalova mchompalova force-pushed the fix/add-test-rds-dsn-prefix branch from 6346114 to b9dcd81 Compare June 1, 2020 14:07
@mchompalova mchompalova requested review from karsov and gkazakov111 June 2, 2020 06:33
@mchompalova mchompalova force-pushed the fix/add-test-rds-dsn-prefix branch from b9dcd81 to 4f405c7 Compare June 2, 2020 06:36
@epavlova
Copy link
Collaborator

epavlova commented Jun 2, 2020

"TEST_RDS_DSN_PREFIX" and "DB_TEST_URL" have the same meaning. So let's fix this. I suggest the following steps:

  1. In this PR - let's rename "TEST_RDS_DSN_PREFIX" to "TEST_DB_DSN" to be more generic and to reflect the real meaning of its value.
  2. Fix "concept-events-notifications" to use this new variable
  3. Fix "generic-rw-aurora" to use this new variable
  4. Remove "DB_TEST_URL" from the orb.

If we know for sure that both environment variables have the same meaning, why we should add the new one. Couldn't we just use DB_TEST_URL in the concepts-events-notifications so we don't have to touch the orb or generic-rw-aurora? DB_TEST_URL is general enough.

epavlova
epavlova requested changes Jun 2, 2020
View reviewed changes
Copy link
Collaborator

@epavlova epavlova left a comment

Choose a reason for hiding this comment

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

What is the reason not to use the current parameter db-test-url?

@mchompalova mchompalova force-pushed the fix/add-test-rds-dsn-prefix branch from 4f405c7 to 452da32 Compare June 3, 2020 07:55
@mchompalova mchompalova requested a review from epavlova June 3, 2020 10:41
@mchompalova
Copy link
Contributor Author

generic-rw-aurora builds successfully with this version.

@mchompalova
Copy link
Contributor Author

mchompalova commented Jun 3, 2020 •
edited
Loading

What is the reason not to use the current parameter db-test-url?

I thought we need 2 params because of generic-rw-aurora. Actually, the generic-rw-aurora build passes with the default value, which is "root:password@/". I updated the orb.

@epavlova
Copy link
Collaborator

epavlova commented Jun 4, 2020

What is the reason not to use the current parameter db-test-url?

I thought we need 2 params because of generic-rw-aurora. Actually, the generic-rw-aurora build passes with the default value, which is "root:password@/". I updated the orb.

Great, than do we need to keep mysql-database parameter with default value pac? Otherwise I'm okay with approving the PR.

@karsov
Copy link

karsov commented Jun 4, 2020

If we know for sure that both environment variables have the same meaning, why we should add the new one. Couldn't we just use DB_TEST_URL in the concepts-events-notifications so we don't have to touch the orb or generic-rw-aurora? DB_TEST_URL is general enough.

I wrote this thinking that we are not going to fix all services now.

What is the reason not to use the current parameter db-test-url?

The variable name is misleading. It is not a URL, it is a DSN. But it may be easier to reuse the variable.
@epavlova

@mchompalova mchompalova force-pushed the fix/add-test-rds-dsn-prefix branch 3 times, most recently from af687b0 to 41fe777 Compare June 4, 2020 15:37
@epavlova
Copy link
Collaborator

epavlova commented Jun 5, 2020

I don't think that fixing the variable name is significant enough for releasing major version of the orb.

@mchompalova mchompalova force-pushed the fix/add-test-rds-dsn-prefix branch from 41fe777 to 1aaee62 Compare June 5, 2020 09:55
@mchompalova mchompalova merged commit b3a43db into master Jun 9, 2020
@mchompalova mchompalova deleted the fix/add-test-rds-dsn-prefix branch June 9, 2020 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epavlova epavlova epavlova approved these changes

@karsov karsov karsov approved these changes

@gkazakov111 gkazakov111 gkazakov111 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mchompalova @epavlova @karsov @gkazakov111