Fix postgresql database naming convention

[marksiemers]

Description of the Change

This fixes the issue that if an app name has a dash "-" in it, then an invalid database name will be generated for postgresql.

Alternate Designs

Some alternative choices:

• Treat the database name consistently, and have this convention apply to all database types
• Refactor to have the database name generation take place in one spot, instead of across many files

Adopting the first would make the second easier to accomplish.

Benefits

Users will not face errors generating a new postgresql database when their app name has a dash in it. Fixes #329

Possible Drawbacks

As written, this will create inconsistent database names between postgresql and other databases.

Also, I changed the name of the TESTING_APP from test_app to sample-app. It didn't cause any pre-existing tests to fail, so I don't think it is an issue.

[marksiemers]

I did not make any changes to the default user for postgresql. It is still root in one or two places.

I can incorporate that change also if wanted.

[drujensen]

@marksiemers yes, go ahead and change the default user.

[marksiemers]

@drujensen - I can get that change in tomorrow. Overall, is my approach of adding a lot of LOC to the test suite acceptable? Not just for this change, but in general?

[marksiemers]

The changes that were merged in from master may mean I have to rework some code. A few questions:

1. With the change to spec/spec_helper.cr is it now preferred not to Dir.cd in any of the tests?
2. Is it preferred to resolve conflicts then create additional commits, or rebase locally, and push all new commits?

[drujensen]

@marksiemers the test coverage and approach your taking looks good to me.

[marksiemers]

Conflicts have been resolved, anything else needed for this PR?

[marksiemers]

One question - should the database name be treated the same for all databases? Should we gsub('-', '_') for sqlite and mysql also?

[drujensen]

@marksiemers yes, I think you can rename it for all 3 for consistency.

[marksiemers]

@drujensen - The naming is now consistent. This one should be good to merge.

[drujensen]

lgtm

[elorest]

@marksiemers In regard to your last question. I think we should always default database names to be underscore instead of hyphen. Same for files and folder names as well.

[elorest]

Why this change? I personally prefer "test_app" since that's what it is.

[marksiemers]

Two reasons, though this might not be the best place for it:

1. In order to test this fix, the app name needs a dash (-) in it
2. This test would be a false green for an app containing "test"

[elorest]

Didn't these already exist? Why does it show you adding them?

[marksiemers]

AFAIK, not for the environment yaml files. It did exist for others:

  def db_yml
    YAML.parse(File.read("#{TESTING_APP}/config/database.yml"))
  end

  def amber_yml
    YAML.parse(File.read("#{TESTING_APP}/.amber.yml"))
  end

  def shard_yml
    YAML.parse(File.read("#{TESTING_APP}/shard.yml"))
  end

[drujensen]

@elorest @eliasjpr Is this approved? Would like to merge it.

[marksiemers]

@elorest

I think we should always default database names to be underscore instead of hyphen.

That should be the case after this is merged in, all 3 dbs will use database_name_base which gsubs "-" with "_"

Same for files and folder names as well.

In #330 we are consistent with underscore, but I did not gsub hyphens. It should be easy enough to do - likely one line (not including tests).

[marksiemers]

The build finally passed, are we good to merge?