Skip to content
This repository has been archived by the owner on Nov 14, 2020. It is now read-only.

postgresql_role: add optional attribute to set search_path #93

Merged
merged 6 commits into from
Nov 1, 2019
Merged

postgresql_role: add optional attribute to set search_path #93

merged 6 commits into from
Nov 1, 2019

Conversation

Hjdskes
Copy link
Contributor

@Hjdskes Hjdskes commented Aug 31, 2019

This PR adds a new attribute to postgresql_role that allows one to set the search path of the new role. Let me know if there's anything I missed or can improve!

@Hjdskes
Copy link
Contributor Author

Hjdskes commented Sep 1, 2019

Just found #90, not sure how I didn't find that first. Anyway, I see there are some things that can be improved, but this should be a good starting point. @dhermes, if you have code laying around, could we compare notes and merge our solutions?

@Hjdskes Hjdskes force-pushed the searchpath branch 2 times, most recently from b3d6709 to 3dd0db8 Compare September 1, 2019 09:52
@Hjdskes
Copy link
Contributor Author

Hjdskes commented Sep 1, 2019

Reading more into #90, I believe our implementations are similar but I am lacking proper handling for all the edge cases of schema names.

Also, my implementation currently runs ALTER ROLE ... SET search_path TO ... unconditionally. In the case where the searchpath attribute has not been set, it becomes TO DEFAULT (which is equal to "$user",public). Seems like this needs to be taken care of better as well, but I'm not sure how yet.

@Hjdskes Hjdskes mentioned this pull request Sep 13, 2019
Closed
@cyrilgdn
Copy link
Contributor

Hi @Hjdskes ,

Thanks for opening this PR and for your work ! Sorry I did not answer earlier, I was a bit out of open source stuff last weeks (who said holidays :) ? )

I'll take a look at your PR in the next days.

@cyrilgdn cyrilgdn self-requested a review September 17, 2019 14:41
cyrilgdn
cyrilgdn suggested changes Sep 24, 2019
View reviewed changes
Copy link
Contributor

@cyrilgdn cyrilgdn left a comment

Choose a reason for hiding this comment

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

First round of remarks. I'll try to test a bit more.

Regarding your remark:

I am lacking proper handling for all the edge cases of schema names.

I'm not against starting with an easy way by not managing corner case as soon as it's clear for the user.

So could you add a validation on the schemas names to assert that there's no corner cases in their names and display a clear error to the user otherwise?

postgresql/resource_postgresql_role.go Outdated Show resolved Hide resolved
postgresql/resource_postgresql_role.go Outdated Show resolved Hide resolved
postgresql/resource_postgresql_role.go Outdated Show resolved Hide resolved
postgresql/resource_postgresql_role.go Outdated Show resolved Hide resolved
postgresql/resource_postgresql_role.go Show resolved Hide resolved
postgresql/resource_postgresql_role.go Outdated Show resolved Hide resolved
@Hjdskes
Copy link
Contributor Author

Hjdskes commented Sep 26, 2019

Thanks for getting back to me! I'll process your review this weekend, hopefully.

@Hjdskes
Copy link
Contributor Author

Hjdskes commented Sep 28, 2019

I have pushed a new commit that fixes your comments @cyrilgdn. I still have to add a simple validation, which I will do in a separate commit. I can squash those later, if preferred.

@Hjdskes
Copy link
Contributor Author

Hjdskes commented Sep 28, 2019

Rebased on current master

@Hjdskes Hjdskes force-pushed the searchpath branch 4 times, most recently from c9921bd to a910e00 Compare September 28, 2019 09:16
@Hjdskes
Copy link
Contributor Author

Hjdskes commented Sep 28, 2019

I've added a limited form of input validation that checks for the presence of the substring ", " in values for the search path. I'm not sure how to write a test for this, please advice if you want to see such a test added. I'm also not sure if this is the best place to add the validation.

postgresql_role: add input validation for commas in search_path
52cf7cb 
Since we split Postgres' output on commas (specifically, ", "), input values for
the search path may not contain this substring. This is legal in Postgres,
however, so we need to check for it.
@navi86
Copy link

navi86 commented Oct 16, 2019

@Hjdskes
I have created PR to your branch with working tests and update logic of updating search path.
Hjdskes#1

@Hjdskes
Copy link
Contributor Author

Hjdskes commented Oct 17, 2019

Thanks! I have responded in the PR itself.

@Hjdskes
Copy link
Contributor Author

Hjdskes commented Oct 18, 2019

I have merged @navi86's PR. @cyrilgdn, please take another look :)

@Hjdskes Hjdskes requested a review from cyrilgdn October 18, 2019 19:54
cyrilgdn
cyrilgdn approved these changes Nov 1, 2019
View reviewed changes
Copy link
Contributor

@cyrilgdn cyrilgdn left a comment

Choose a reason for hiding this comment

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

It seems to work well, thanks a lot 👍 🎉

@cyrilgdn cyrilgdn merged commit 9109621 into hashicorp:master Nov 1, 2019
@Hjdskes Hjdskes deleted the searchpath branch November 1, 2019 14:08
@Hjdskes
Copy link
Contributor Author

Hjdskes commented Nov 1, 2019

Awesome, thank you!

@cyrilgdn
Copy link
Contributor

cyrilgdn commented Nov 4, 2019

@Hjdskes v1.3.0 has been released on Friday with this search_path feature 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cyrilgdn cyrilgdn cyrilgdn approved these changes

Assignees
No one assigned
Labels
documentation size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Hjdskes @cyrilgdn @navi86