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

(maint) Fix to grant_spec.rb regarding database test on earlier machines #966

Merged
merged 1 commit into from
Mar 8, 2018
Merged

(maint) Fix to grant_spec.rb regarding database test on earlier machines #966

merged 1 commit into from
Mar 8, 2018

Conversation

david22swan
Copy link
Member

@david22swan david22swan commented Mar 6, 2018

The database test has issues on machines with postgresql version's prior to '9.1.24'.
This is due to the fact that the functions implement by '#891', i.e. 'ensure => absent uses REVOKE instead of GRANT' was not implemented in postgresql databases until this version. No issue has arisen with the use of revoke on separate tables however so the test's involving them have been left alone.
This PR introduces an exemption on the specified test that prevent's it from being run against machines that do not support it and a warning has been included within the readme file.

@pmcmaw
Copy link
Contributor

pmcmaw commented Mar 6, 2018

Can you update the title to a more meaningful title and description.
The current one would make it very difficult to write a changelog entry.

@@ -1154,7 +1154,7 @@ Manages grant-based access privileges for users, wrapping the `postgresql::serve

##### `ensure`

Specifies whether to grant or revoke the privilege. Default is to grant the privilege.
Copy link
Contributor

Choose a reason for hiding this comment

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

This may require more information that stating it will create problems. Jean will give you some help writing this section or ensure to get it reviewed by Jean.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great update. We will get it reviewed by Jean to ensure its ok. I would add that revoke works on tables, the limit is only on revoke permissions on the database or something along them lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would change this to:

Specifies whether to grant or revoke the privilege. The value 'present' grants the privilege, and 'absent' revokes the privilege. Revoke or 'absent' works only in PostgreSQL version 9.1.24 or later.

@pmcmaw pmcmaw requested a review from jbondpdx March 6, 2018 15:59
@pmcmaw
Copy link
Contributor

pmcmaw commented Mar 6, 2018

Hey @jbondpdx,

Jus wondering if you could provide some guidance on the README update for this change.
Long story short, Revoke permissions work on tables but it was only implemented to the database in a later version. The docs associated can be found here.

@david22swan david22swan changed the title grant_spec.rb fix Fix to grant_spec.rb. The database test has issues on machines with postgresql version's prior to '9.1.24'. Mar 7, 2018
@david22swan david22swan changed the title Fix to grant_spec.rb. The database test has issues on machines with postgresql version's prior to '9.1.24'. Fix to grant_spec.rb regarding database test on earlier machines Mar 7, 2018
@david22swan david22swan changed the title Fix to grant_spec.rb regarding database test on earlier machines (maint) Fix to grant_spec.rb regarding database test on earlier machines Mar 7, 2018
Copy link
Contributor

@jbondpdx jbondpdx left a comment

Choose a reason for hiding this comment

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

Fix for the noted line.

@david22swan
Copy link
Member Author

@jbondpdx Is this fine???

Copy link
Contributor

@jbondpdx jbondpdx left a comment

Choose a reason for hiding this comment

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

Since you specify below what the default is and what each value does, I think we can delete the bit in the paragraph about what the default does. So the final should be:

Specifies whether to grant or revoke the privilege. Revoke or 'absent' works only in PostgreSQL version 9.1.24 or later.

@david22swan
Copy link
Member Author

@jbondpdx Forgot about that, fixed :)

@pmcmaw
Copy link
Contributor

pmcmaw commented Mar 8, 2018

Nice one David.

@pmcmaw pmcmaw merged commit babe5e4 into puppetlabs:master Mar 8, 2018
@david22swan david22swan deleted the ensure_fix branch July 17, 2018 09:27
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.

4 participants