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

feat: added db:info command #1172

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

reilg
Copy link

@reilg reilg commented Oct 6, 2022

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

Description of change

Adds the db:info command which displays the config information.

@WikiRik
Copy link
Member

WikiRik commented Oct 13, 2022

Thanks for the PR! I haven't had the time to take a look at this yet, but I haven't forgotten about it

@WikiRik
Copy link
Member

WikiRik commented Oct 23, 2022

As further context, before I have to search for it again in Slack;

This PR was first opened as #931

Suggestions from Slack, not very relevant - A suggestion was made there to 'make the output a little prettier' - There the suggestion was made to log it line by line instead of logging the object itself.
// Your example
Config {
  username: 'root',
  password: '***',
  database: 'test_database',
  host: 'localhost',
  port: '3306',
  dialect: 'mysql'
}

// Suggestion
Sequelize configuration:
username: root
password: ***
database: test_database
host: localhost
port: 3306
dialect: mysql
  • An additional suggestion on Slack was to add a --raw flag so the output would still be the object
  • A note added there was that the configuration can have nested objects and that those should work with this as well. A proposal for that was;
define.underscored: true
some.other.nested.value: value here
  • The config used for the CLI tests would probably be used to test this new feature since it also has nested objects.

  • Additionally a --show-password flag was proposed to show the password.

The tests that should be added here should use simplified versions of the test config and check the output is as expected. Suggested tests are;

  1. With a very basic config, verify that the output by using the command is correct
  2. With a very basic config, verify that the output by using the --raw flag is correct
  3. With a config with nesting, verify that the output by using the command is correct
  4. With a config with nesting, verify that the output by using the --raw flag is correct
  5. With a very basic config, verify that the output by using the --show-password flag is correct

These tests can be based on https://github.com/sequelize/cli/blob/main/test/version.test.js
One test that specifies a different config location that can be used is https://github.com/sequelize/cli/blob/main/test/options.test.js#L50-L66
Seeing that most/all tests should be run on both configs, something like the following could be useful to keep in mind; https://github.com/sequelize/cli/blob/main/test/db/seed/all.test.js#L7-L17

@reilg do you think you can work on this further with this information?

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