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

Restructuring dbnames #170

Merged
merged 3 commits into from
Feb 21, 2019
Merged

Restructuring dbnames #170

merged 3 commits into from
Feb 21, 2019

Conversation

kadrlica
Copy link
Collaborator

@kadrlica kadrlica commented Feb 8, 2019

This PR is meant to allow access to decade. It originally sought to:

  • Add decade to the list of allowed databases.
  • To define the list of allowed databases in one place (Unique list of allowed dbnames #169)
    However, after some consideration, the restriction on database names was removed from the parser.

@mgckind
Copy link
Owner

mgckind commented Feb 15, 2019

can we merge this?

@mgckind
Copy link
Owner

mgckind commented Feb 15, 2019

want me to jump in? pinging @Michael-D-Johnson so he is aware of the conversation

@kadrlica
Copy link
Collaborator Author

I wasn't able to test (didn't have access yet). Also, I thought we might want to use this opportunity to centralize the list of databases.

@kadrlica
Copy link
Collaborator Author

kadrlica commented Feb 15, 2019

More generally, why are we tracking the viable databases at all? Shouldn't the code just check the services file directly? If a database exists in the services file then try to connect, otherwise throw an exception?

@kadrlica
Copy link
Collaborator Author

kadrlica commented Feb 16, 2019

If we don't restrict database names at the parser level, this is what we get:

Database entered is not in ['db-dessci', 'db-desdr', 'db-destest', 'db-desoper'] or in DES_SERVICE file, continue anyway [y]/n

Adding section db-test to DES_SERVICES file

Enter username : hello
Enter password : 

ORA-12514: TNS:listener does not currently know of service requested in connect descriptor

I'll add a bit more descriptive message.

@kadrlica
Copy link
Collaborator Author

@mgckind ok, I think this is ready to merge and tag.

@mgckind
Copy link
Owner

mgckind commented Feb 18, 2019

Thanks, will take a look

@kadrlica
Copy link
Collaborator Author

kadrlica commented Feb 21, 2019

@mgckind we have a DELVE meeting on Friday. Do you think this new version could be live by then?

@mgckind
Copy link
Owner

mgckind commented Feb 21, 2019

yes, will do soon

@mgckind mgckind merged commit 81d6a77 into master Feb 21, 2019
@stringkm stringkm mentioned this pull request May 1, 2019
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