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 yum::copr resource to handle COPR repositories. #215

Merged
merged 5 commits into from
Jun 26, 2023

Conversation

olifre
Copy link

@olifre olifre commented Jun 10, 2021

COPR (Cool Other Package Repo) is a Fedora project for third-party package repositories, see:
https://copr.fedorainfracloud.org/

Pull Request (PR) description

This PR adds support to manage COPR repositories via a new yum::copr resource.
Documentation and test are included.

This Pull Request (PR) fixes the following issues

Fixes #149

Copy link

@traylenator traylenator 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 a good case for an acceptance test as well I'd say.

# }
#
define yum::copr (
String $copr_repo = $title,

Choose a reason for hiding this comment

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

Variable $repo is proably enough since it's already in yum::copr type.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, I changed that in the last force push 👍 .

Enum['enabled', 'disabled', 'removed'] $ensure = 'enabled',
) {
$prereq_plugin = $facts['package_provider'] ? {
'dnf' => 'dnf-plugins-core',

Choose a reason for hiding this comment

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

I always prefer to make the future, dnf in this case, the default case since it's more obvious what code needs removing once yum is gone from the universe.

Copy link
Author

Choose a reason for hiding this comment

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

That's a good idea, thanks for the suggestion! I've also addressed this in the last push.

@olifre
Copy link
Author

olifre commented Jun 28, 2021

This is a good case for an acceptance test as well I'd say.

Indeed it is. I've addressed all comments apart from the acceptance test, will look into this later this week.

# }
#
define yum::copr (
String $repo = $title,
Copy link
Member

Choose a reason for hiding this comment

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

please add puppet-strings like documentation for the new parameters

Copy link
Author

Choose a reason for hiding this comment

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

Done in the last force-push, really appreciate all the feedback from review, since I'm not yet used to most of the scaffolding and test infrastructure, and you are pushing me to read into the correct things 👍 .

@bastelfreak bastelfreak added the enhancement New feature or request label Jun 28, 2021
@olifre olifre force-pushed the add-copr-support branch 2 times, most recently from b152e50 to 5a4360b Compare June 28, 2021 18:15
@olifre
Copy link
Author

olifre commented Jun 28, 2021

I've now fixed the Rubocop offenses in the existing test, and added an acceptance test. I don't have a box at hand (yet) to run the acceptance test, but I think it should work — CI will show ;-).

@olifre
Copy link
Author

olifre commented Jun 28, 2021

Nice, the acceptance test found a real bug: disableing a COPR repository without adding it first will fail, and not be idempotent on dnf-based systems (which leave the repository file in place, but switch it to disabled in this case). yum-based systems effectively don't differentiate between disabled or removed.

I have fixed that in the last force-push by ensuring a COPR repository is added before disabling it, if disabled is requested and it is not disabled yet on dnf-based systems.

Another issue was that the removed acceptance test did not add a repository first, so it was not a real test whether the removing worked. This is now also fixed.

So I hope this might be the last iteration :-).

@traylenator
Copy link

One of the problems with this if you are running with

resources{'yumrepo':
   purge => true,
}

The repo file will be deleted won't it?

I don't see a good way around that.

@olifre
Copy link
Author

olifre commented Jun 29, 2021

The repo file will be deleted won't it?

Indeed...

I don't see a good way around that.

Same here, at least not as long as we want to use the official way (via dnf / yum) instead of managing the repo "manually", but then all the magic hidden by the underlying package manager plugins is lost and we'd need to follow all updates on that end.

Good ideas are very welcome.

@olifre
Copy link
Author

olifre commented Feb 8, 2023

Sorry for the multiple force-pushes, I found a small logic issue in the last commit and fixed it — if there is any good proposal on how to upstream this, fixing / documenting the issue @traylenator pointed out, please let me know.

@bastelfreak
Copy link
Member

@olifre force pushes are fine. Can you take a look at the failing CI jobs?

@olifre olifre force-pushed the add-copr-support branch 2 times, most recently from 792e781 to d1902a1 Compare February 8, 2023 19:30
@olifre
Copy link
Author

olifre commented Feb 8, 2023

@olifre force pushes are fine. Can you take a look at the failing CI jobs?

Thanls, I think I fixed all CI issues, the remaining issue seems to be a fluke of the GitHub actions runner (read timeout reached). I will trigger CI once more with an empty force-push.

@olifre
Copy link
Author

olifre commented Feb 8, 2023

@bastelfreak That did the trick, now all is green 😄 .

@olifre
Copy link
Author

olifre commented Feb 10, 2023

@bastelfreak It seems CI went red again after you re-ran it yesterday, but checking the logs, that seems like an issue with the runners, correct?

@traylenator
Copy link

traylenator commented May 24, 2023

One of the problems with this if you are running with

resources{'yumrepo':
   purge => true,
}

The repo file will be deleted won't it?

I don't see a good way around that.

Given there's no sensible way around this and it only affects particular configurations I would just document it.
If the resources declaration can be adjusted to ignore the copr repos based on name or something then makes sense to give that example.

https://forge.puppet.com/modules/crayfishx/purge/readme gives an example in fact.

@olifre
Copy link
Author

olifre commented May 29, 2023

Given there's no sensible way around this and it only affects particular configurations I would just document it.
If the resources declaration can be adjusted to ignore the copr repos based on name or something then makes sense to give that example.

Thanks for confirming!

The names of yumrepo resources resulting from COPR commands end up being formed such as:
copr:copr.fedorainfracloud.org:jdoss:wireguard
copr:copr.fedorainfracloud.org:copart:restic
(tested with RHEL 7 and 8). Since resources itself does not seem to allow limiting what is purged, the only way out seems to be to use something like the purge module you linked.

What do you think about the documentation addendum I added in the preceding commit?

@olifre olifre force-pushed the add-copr-support branch 2 times, most recently from 0675c06 to 5da2f01 Compare June 25, 2023 18:04
@olifre
Copy link
Author

olifre commented Jun 25, 2023

I have finally come around to also regenerate REFERENCE.md and rebase onto current master, so things should now be green and mergable.

'dnf' => 'dnf-plugins-core',
default => 'yum-plugin-copr',
}
stdlib::ensure_packages([$prereq_plugin])
Copy link
Member

Choose a reason for hiding this comment

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

please introduce a new param like $manage_prereq_plugin and wrap an if condition around thr function.

Copy link
Author

Choose a reason for hiding this comment

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

That makes a lot of sense, done and force-pushed.

@bastelfreak bastelfreak merged commit 6822a42 into voxpupuli:master Jun 26, 2023
@olifre olifre deleted the add-copr-support branch June 26, 2023 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] COPR support
3 participants