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

Add new common repo which contains add-ons #1190

Merged
merged 1 commit into from
Apr 12, 2021

Conversation

jorhett
Copy link
Contributor

@jorhett jorhett commented Sep 3, 2020

Detailed in https://yum.postgresql.org/news/new-repo-rpms-released/, many add-ons and extensions were moved to a common repo. Without this common repo certain addon installations will fail.

@jorhett jorhett requested a review from a team as a code owner September 3, 2020 06:38
@puppet-community-rangefinder
Copy link

postgresql::globals is a class

Breaking changes to this file WILL impact these 19 modules (exact match):
Breaking changes to this file MAY impact these 1 modules (near match):

postgresql::repo is a class

that may have no external impact to Forge modules.

postgresql::repo::yum_postgresql_org is a class

that may have no external impact to Forge modules.

This module is declared in 71 of 575 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

Copy link
Contributor

@sanfrancrisko sanfrancrisko 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 the enhancement @jorhett - LGTM. Unfortunately, at the moment there is an issue with our tests on the CentOS 8 containers which we're trying to resolve at the moment. Once we're operational again, I'll re-kick the tests and merge.

@igalic
Copy link
Contributor

igalic commented Sep 22, 2020

I'm finding the placement or the naming a bit odd, given that this only applies to RHEL systems

@jorhett
Copy link
Contributor Author

jorhett commented Sep 23, 2020

I'm finding the placement or the naming a bit odd, given that this only applies to RHEL systems

Yes, _commonurl seems odd but changing _baseurl would be a breaking change so I went with the minimal change. If you want a complete fix we should set baseurl to the repo top-level location, and then added common and pgdg as suffixes to that path. I'm happy to submit it, but it will be a breaking change :(

Even better alternative is for us to install the repos RPM package and allow them to adjust their repos as they see fit 👍

I'm happy to go whatever way you want, but doing nothing is not an option since existing uses of this module do not work now that binaries have been moved out of the single repo https://www.postgresql.org/about/news/2027/

@sanfrancrisko
Copy link
Contributor

@jorhett , @igalic One possible suggestion might be to rename the $repo_commonurl parameter to $yumrepo_commonurl (or $yum_repo_commonurl) given this exclusively pertains to the PG YUM repos? We could also update the parameter description so it's a bit more obvious in the REFERENCE.md:

Sets the url for the PostgreSQL common repository. Useful if you host your own mirror of the repository. Applies to RHEL / Fedora systems only.

...although hopefully the parameter name makes it obvious enough.

Also, @jorhett - whenever you get a chance could you rebase this branch with puppetlabs:main - we excluded CentOS 8 test execution temporarily whilst we look in to the issue. We have alternative methods to test CentOS 8 internally, so we're not losing coverage. This seems quite a specific case to that particular Docker container.

@jorhett
Copy link
Contributor Author

jorhett commented Apr 7, 2021

I am amazed that an officially supported Puppetlabs repo is still broken exactly 1 year after this change was made by the postgresql upstream maintainers.

I brought this issue to you, and provided a patch. This fix was held up because you wanted the variable renamed?

Was this a good use of your time and mine? You can rename a variable with a few keystrokes. Bouncing it back to me where it sat for 7 months because I've been too busy on other stuff... while your module is broken. This wasn't lack of tests. This wasn't a problem with the implementation. This was a variable name. And this module has been broken for a full year now...

Anyway, it's really not clear what part you wanted renamed so I made a guess and implemented the change I think you want. Take a look now. And if you think another variable should be renamed, how about you just do that?

@jorhett jorhett force-pushed the add_common_yumrepo branch from ca4e32a to 13d7fe8 Compare April 7, 2021 03:38
@sanfrancrisko
Copy link
Contributor

@jorhett Apologies for this languishing for so long. This fell through the cracks and shouldn't have been held up for as long as it was. Given there were other parties interested in the PR, and parameter naming can sometimes be surprisingly contentious, the intention was to leave this for a week or two max to ensure no other comments about naming landed in, then just rename and merge if nobody came back and it was still open.

For my part, apologies I didn't get returning to this sooner than I did - similar to yourself, many other things were on my plate. Regardless, the IAC Team now triage open PRs on Mondays, so we really shouldn't have gotten in to this position. I have a feeling how this blind spot emerged in our process and I'm going to chat with the team this week about it so we don't hit this again.

Thanks again for the contribution and apologies for the delay in getting it landed.

@sanfrancrisko sanfrancrisko merged commit 289aab3 into puppetlabs:main Apr 12, 2021
@sanfrancrisko
Copy link
Contributor

@jorhett This is now in v7.1.0 of the module which is live on the Forge now

@jorhett jorhett deleted the add_common_yumrepo branch April 15, 2021 05:37
@jorhett
Copy link
Contributor Author

jorhett commented Apr 15, 2021

Thank you. Numerous teams are happy to stop running local forks 👍

I have a feeling how this blind spot emerged in our process

Of course that never happens to me 🤣 so I ain't gonna throw any more stones from my glass house 😆

cegeka-jenkins pushed a commit to cegeka/puppet-postgresql that referenced this pull request Feb 3, 2022
Add new common repo which contains add-ons
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants