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

remote-state/pg: skip schema creation instruction if exists #21607

Merged
merged 1 commit into from
Aug 27, 2019

Conversation

yanndegat
Copy link
Contributor

  • add skip_schema_creation option
  • add sanity check to avoid situations where postgres users
    hasn't been granted the "CREATE SCHEMA" right

closes #21604

Signed-off-by: yann degat yann@2kmail.net

Copy link
Contributor

@pselle pselle left a comment

Choose a reason for hiding this comment

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

This is looking good! Question: the current test that is added covers if the schema already exists and the setting is set to true -- what will happen (and should we have a test to cover?) if the schema does not exist and the setting is set to true?

website/docs/backends/types/pg.html.md Outdated Show resolved Hide resolved
backend/remote-state/pg/backend.go Outdated Show resolved Hide resolved
backend/remote-state/pg/backend.go Outdated Show resolved Hide resolved
backend/remote-state/pg/backend.go Outdated Show resolved Hide resolved
@yanndegat
Copy link
Contributor Author

This is looking good! Question: the current test that is added covers if the schema already exists and the setting is set to true -- what will happen (and should we have a test to cover?) if the schema does not exist and the setting is set to true?

hi @pselle

thanks a lot for your review
i'm quite disappointed because what you suggest (test the case when it should fail) seemed to be very easy to impl at first glance, but in fact i'm quite stuck because the whole logic is done
into the backend/testing.go:TestBackendConfig func

and inside this function, theres a call to t.Fatal(), which i cant "recover()"
do you know any way to catch this error other than duplicating the whole TestBackendConfig into my unit test ?

@pselle pselle requested a review from jbardin June 7, 2019 19:18
@pgaxatte
Copy link

pgaxatte commented Jul 8, 2019

Hello,

Any news on this PR?
It seems to solve an issue of mine too.

@jbardin
Copy link
Member

jbardin commented Jul 8, 2019

@mars, Just verifying if this looks OK for you too. Thanks!

Copy link
Contributor

@mars mars left a comment

Choose a reason for hiding this comment

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

Great to see this improvement @yanndegat 🙌

I left a few suggestions that are not blockers but would be great to see accepted into the PR.

website/docs/backends/types/pg.html.md Outdated Show resolved Hide resolved
backend/remote-state/pg/backend.go Outdated Show resolved Hide resolved
* add `skip_schema_creation` option
* add sanity check to avoid situations where postgres users
  hasn't been granted the "CREATE SCHEMA" right

closes hashicorp#21604

Signed-off-by: yann degat <yann@2kmail.net>
@yanndegat
Copy link
Contributor Author

hello @mars & @jbardin

thanks a lot for your reviews

i've just taken your suggestions into account an pushed them

thanks again

regards

@yanndegat
Copy link
Contributor Author

hi @mars & @jbardin

any news on this PR?

thanks again

Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look, @mars , and for making the suggested updates @yanndegat !

@mildwonkey mildwonkey merged commit be5280e into hashicorp:master Aug 27, 2019
@ghost
Copy link

ghost commented Sep 27, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Sep 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remote state/pg: skip schema creation option
7 participants