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

Updating the --no-comment argument to the correct --no-comments argument #44028

Merged
merged 1 commit into from Jan 2, 2022

Conversation

aldent95
Copy link
Contributor

Summary

Change to fix an incorrect argument in the PostgreSQL structure dump tasks.
The current argument of --no-comment isn't correct and causes an error anytime it's used with pg_dump

Other Information

See full info about the bug in issue #44027

@MatheusRich
Copy link
Contributor

@aldent95 no tests were failing because of this error? Can we write a test that would have failed in this case?

@aldent95
Copy link
Contributor Author

aldent95 commented Jan 1, 2022

@aldent95 no tests were failing because of this error? Can we write a test that would have failed in this case?

There were tests that I've updated in this PR. But that relies on the correct argument being in the test as well. The only way to truly test it would be to actually have tests that run pg_dump itself.
Being a first time contributor I don't have enough knowledge of the CI/CD Rails uses to know if that's even possible.

@eugeneius eugeneius linked an issue Jan 2, 2022 that may be closed by this pull request
@eugeneius eugeneius merged commit 8b29524 into rails:main Jan 2, 2022
eugeneius added a commit that referenced this pull request Jan 2, 2022
Updating the --no-comment argument to the correct --no-comments argument
@eugeneius
Copy link
Member

Backported to 7-0-stable in c837883. Thanks!

@ghiculescu
Copy link
Member

@aldent95 no tests were failing because of this error? Can we write a test that would have failed in this case?

There were tests that I've updated in this PR. But that relies on the correct argument being in the test as well. The only way to truly test it would be to actually have tests that run pg_dump itself. Being a first time contributor I don't have enough knowledge of the CI/CD Rails uses to know if that's even possible.

#44069 implements this idea.

ghiculescu added a commit to ghiculescu/rails that referenced this pull request Jan 4, 2022
rails#44028 came about because all the current Postgres tests just mock the `pg_dump` call made when you dump a DB structure.

By actaully running a dump at least once, we can test against a real database and ensure that all arguments are correctly handled. This PR implements that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ActiveRecord - Postgres Structure dump uses wrong arg
5 participants