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

[PACKAGES] Add support for FreeBSD & OpenBSD #77

Merged
merged 5 commits into from
Jun 8, 2020

Conversation

ingrinder
Copy link
Collaborator

@ingrinder ingrinder commented Jun 5, 2020

Description

Simply adds the OpenBSD and FreeBSD package managers to the entry so it works.

Reason and / or context

See #69.

How has this been tested ?

On fresh virtual machines.

Types of changes :

  • Bug fix (non-breaking change which fixes an issue)
  • Typo / style fix (non-breaking change which improves readability)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist :

  • [IF NEEDED] I have updated the README.md file accordingly ;
  • [IF NEEDED] I have updated the test cases (which pass) accordingly ;
  • My changes looks good ;
  • I agree that my code may be modified in the future ;
  • My code follows the code style of this project (PEP8).

@ingrinder ingrinder added the enhancement ⬆️ Implements a new feature, fixes or improves existing ones label Jun 5, 2020
@ingrinder
Copy link
Collaborator Author

Oh! Tests! Oops... It's about time I go to bed now, let me convert this to draft and I'll fix that tomorrow 😄

@ingrinder ingrinder marked this pull request as draft June 5, 2020 02:20
@ingrinder ingrinder changed the title [PACKAGES] Add FreeBSD/OpenBSD support. [WIP] [PACKAGES] Add FreeBSD/OpenBSD support. Jun 5, 2020
@HorlogeSkynet HorlogeSkynet added this to the v4.8.0 milestone Jun 5, 2020
@HorlogeSkynet
Copy link
Owner

HorlogeSkynet commented Jun 5, 2020

No worries ! I took the liberty to simplify writing tests for this entry, hoping it will (DRY and) help you add the missing ones tonight 😅
I'm pre-accepting the changes so you can squash the whole thing up when you'll be ready without me.
Bye Michael, have a nice WE 👌

@HorlogeSkynet HorlogeSkynet marked this pull request as ready for review June 5, 2020 13:51
@ingrinder ingrinder mentioned this pull request Jun 5, 2020
13 tasks
@ingrinder
Copy link
Collaborator Author

ingrinder commented Jun 5, 2020

Ah... I just rewrote the package tests for this before pulling your changes... We made very, very similar changes 😆

I've pushed them to bsd_support/packages_alt, perhaps we could merge them together?

Edit: To be honest, I think I prefer your approach with seperate cases, so if you want we can merge this in and just delete the other branch.

Meanwhile, I'll add the required tests here too 👍

Edit 2: Also, you too! Catch you later 😄

@HorlogeSkynet
Copy link
Owner

Oh sorry... There was some concurrency in our flow 🙄
I'll be honest too, I also prefer mine ; no offense.

Although, I might have noticed an inconsistency.
For pkg you've set the skew to 1 but your test output does not contain any, and actually got 7 entries (instead of 6) !
Did you omit a line in the mocked output ?

Thanks, see you 👋

Uses more-correct mocks for `pkg` and `pkg_info`.
@ingrinder
Copy link
Collaborator Author

Yeah, sorry about that, I was a bit cheeky and since my BSD installations have ~150 packages each I didn't fancy adding in an extra 300 lines to the test just for those...! Each of those lists was simply the first 6/7 packages listed in each output.

I went about installing two fresh copies and I've now put their far more minimal outputs into the tests so they're more correct 👍. You can see the pkg test correctly ignores pkg itself from the output.

@HorlogeSkynet
Copy link
Owner

Yeah, sorry about that, I was a bit cheeky and since my BSD installations have ~150 packages each I didn't fancy adding in an extra 300 lines to the test just for those...! Each of those lists was simply the first 6/7 packages listed in each output.

No problem ! All the other tests are actually only partial outputs too, to avoid really-really-really-long irrelevant lists...

I went about installing two fresh copies and I've now put their far more minimal outputs into the tests so they're more correct 👍. You can see the pkg test correctly ignores pkg itself from the output.

I'm not sure about ignoring pkg itself 🤔 😂
I mean, if it is an installed package, that may receive updates (or even be removed), why shouldn't we count it ?
The same situation may occur against other package managers, but we actually don't take it into consideration right now 🙄
It's not a major issue, but I think we should take advantage of this to clarify how we are supposed to handle it. Tell me what you think about that !

@ingrinder
Copy link
Collaborator Author

I mean, if it is an installed package, that may receive updates (or even be removed), why shouldn't we count it ?

Ohhh!, I've misinterpeted the comment in packages.py:

# If any, deduct output skew present due to the packages tool.

I thought this meant the package manager itself was deducted from the count - but this actually means any skew due to the formatting of the output, right? So, I assume all the other package managers already include themselves if they are listed? That does make sense, like you say they are indeed packages with versions that can be updated and such. We should probably still treat them as a package in our count, then. Sorry for the mix-up! 😄

Assuming we're following this pattern, that removes the skew from pkg entirely - I'll change it now 😁

@HorlogeSkynet HorlogeSkynet changed the title [WIP] [PACKAGES] Add FreeBSD/OpenBSD support. [PACKAGES] Add support for FreeBSD & OpenBSD Jun 8, 2020
@HorlogeSkynet HorlogeSkynet merged commit 7b49d1c into master Jun 8, 2020
OS, Distributions & Hardware automation moved this from IN PROGRESS to DONE Jun 8, 2020
@HorlogeSkynet HorlogeSkynet deleted the bsd_support/packages branch June 8, 2020 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ⬆️ Implements a new feature, fixes or improves existing ones
Development

Successfully merging this pull request may close these issues.

None yet

2 participants