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

chore(#40): depracate DB_ADMIN_XXX in favor of DB_XXX to aling with #41

Closed
wants to merge 1 commit into from

Conversation

iocanel
Copy link

@iocanel iocanel commented Sep 10, 2018

the rest rdbms playbooks.

@tchughesiv
Copy link
Contributor

tchughesiv commented Sep 10, 2018

@iocanel tks for the contribution. with the other RDBMS offerings, DB_USER is leveraged for unprivileged user accounts. But, w/ mysql for example, there are separate variables for root/admin access.

e.g. https://github.com/ansibleplaybookbundle/mariadb-apb/blob/83eb0ff0026fa0485fb5b0bd148ffa9266d46bf2/templates/deployment.yaml.j2#L54

w/ mssql, the SA account is that equivalent so it feels as if we should reserve the DB_USER variable for a regular user. this "user create" logic is not yet built into the mssql image or apb like it is w/ the other RDBMS offerings. some modifications would first have to be made to the image and apb before switching to a DB_USER var.

@iocanel
Copy link
Author

iocanel commented Sep 18, 2018

So what do u suggest? Remove the deprecation comment and keep both sets around until support for admin password is added?

@iocanel
Copy link
Author

iocanel commented Sep 18, 2018

fwiw, I want to align books as much as possible so that I caneasier maintain: https://github.com/snowdrop/servicecatalog-connector

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.

@iocanel my only concern w/ this approach is that folks may get confused and think they're using an unprivileged user... but I think I'm ok w/ it until additional logic is added to offer separate users types.

@@ -239,6 +239,9 @@
DB_TYPE: mssql
DB_HOST: '{{ service_instance }}.{{ namespace }}.svc'
DB_PORT: '{{ service_port }}'
DB_USER: SA
DB_PASSWORD: '{{ mssql_sa_pw }}'
# deprecated: DB_ADMIN_USER and DB_ADMIN_PASSWORD will be replaced by DB_USER and DB_PASSWORD.
Copy link
Contributor

Choose a reason for hiding this comment

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

@iocanel let's just remove this depreciation comment for now as you suggested.

@iocanel
Copy link
Author

iocanel commented Oct 26, 2022

This is stale for way to long, closing.

@iocanel iocanel closed this Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants