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

renamed provider to fit current status of alternative usage in distributions / Add SLES12/15 support #101

Merged
merged 2 commits into from
Apr 22, 2020

Conversation

pseiler
Copy link
Contributor

@pseiler pseiler commented Dec 13, 2019

Pull Request (PR) description

I renamed the rpm provider for this module as it fits the current distribution usage more. I also tested support for SLES12 and SLES15, which have the same functionality in update-alternatives as debian-based systems have. RHEL based system instead chkconfigs implementation.

Things I've done:

  • renamed 'rpm' to 'chkconfig'
  • added 'osfamily' 'Suse' as default for 'dpkg' provider
    (tested with SLES12 SP4 and SLES15 SP1)
  • updated metadata.json according to new SLES support

@pseiler
Copy link
Contributor Author

pseiler commented Dec 13, 2019

I'll fix the failing travis checks ;)

@pseiler
Copy link
Contributor Author

pseiler commented Dec 13, 2019

@ekohl maybou you'll have a look. Thank you in advance!

@ekohl
Copy link
Member

ekohl commented Dec 13, 2019

I don't really use alternatives so I feel like I'm not very qualified to review.

Copy link
Member

@bastelfreak bastelfreak left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I will leave this open so someone with more knowledge about types/providers and/or alternatives can have a look as well.

@bastelfreak bastelfreak changed the title renamed providers to fit current status of alternative usage in distributions renamed providers to fit current status of alternative usage in distributions / Add SLES12/15 support Dec 15, 2019
@bastelfreak bastelfreak added the enhancement New feature or request label Dec 15, 2019
@vox-pupuli-tasks
Copy link

Dear @pseiler, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

1 similar comment
@vox-pupuli-tasks
Copy link

Dear @pseiler, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@pseiler
Copy link
Contributor Author

pseiler commented Dec 29, 2019

any news regarding this pr?

@ghoneycutt ghoneycutt self-requested a review December 30, 2019 16:09
Copy link
Member

@ghoneycutt ghoneycutt left a comment

Choose a reason for hiding this comment

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

I do not get the motivation to rename things from their technology (rpm, dpkg) to an interpretation of how old they are (current, legacy). This is not the norm with any other set of providers.

Believe that Suse support can be added to existing providers using constraints.

@pseiler
Copy link
Contributor Author

pseiler commented Dec 30, 2019

@ghoneycutt what would be a good solution? I have no idea.

As SUSE based systems use rpm, not dpkg imo the provider name is wrong as SUSE belongs to dpkg based system in this constellation

@pseiler
Copy link
Contributor Author

pseiler commented Jan 17, 2020

ping

1 similar comment
@pseiler
Copy link
Contributor Author

pseiler commented Jan 27, 2020

ping

@pseiler
Copy link
Contributor Author

pseiler commented Feb 19, 2020

Can this be merged or could someone provide me a hint to another solution for this. I'd like to use this code in our productive environment which contain SLES servers.

Without a change like this the puppet run on our SLES servers would fail. And maintaining a fork is also a bad idea.
@ghoneycutt @bastelfreak @ekohl

@pseiler pseiler requested a review from ghoneycutt March 1, 2020 15:49
@pseiler
Copy link
Contributor Author

pseiler commented Apr 1, 2020

ping

@ekohl
Copy link
Member

ekohl commented Apr 2, 2020

I disagree with the naming. RPM based uses https://github.com/fedora-sysv/chkconfig which isn't an old version but really a different flavor. Fedora uses that and it's still receiving updates. In Debian it's provided by dpkg as a util. I don't know what the upstream for Suse is but following the naming of upstreams is probably more correct.

@pseiler
Copy link
Contributor Author

pseiler commented Apr 3, 2020

I also researched again and got the same result. I also checked SUSE distributions and they indeed use the debian tools.
Proof:

rpm -qpi https://download.opensuse.org/distribution/leap/15.1/repo/oss/x86_64/update-alternatives-1.19.0.4-lp151.3.67.x86_64.rpm

Output on a SLES machine:

Name        : update-alternatives
Version     : 1.18.4
Release     : 16.3.1
Architecture: x86_64
[...]
URL         : http://ftp.de.debian.org/debian/pool/main/d/dpkg/
Summary     : Maintain symbolic links determining default commands
Description :
[...]

I now suggest following naming scheme
dpkg (leave as it is because something like (dpkg-utils) includes a hyphen which introduces unwanted file names)
rpm -> chkconfig

Is this okay with you @ekohl. If yes I will update this pr and mention you again?

@vox-pupuli-tasks
Copy link

Dear @pseiler, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

2 similar comments
@vox-pupuli-tasks
Copy link

Dear @pseiler, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@vox-pupuli-tasks
Copy link

Dear @pseiler, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@pseiler pseiler changed the title renamed providers to fit current status of alternative usage in distributions / Add SLES12/15 support renamed provider to fit current status of alternative usage in distributions / Add SLES12/15 support Apr 8, 2020
@pseiler
Copy link
Contributor Author

pseiler commented Apr 8, 2020

@ekohl @ghoneycutt I just changed everything so it fit's our discussion etc. I also updated the introduction message so it won't get confusing. Please have a look and merge it, if everything is fine.

@pseiler
Copy link
Contributor Author

pseiler commented Apr 14, 2020

The travis check was successfull. Mabye some hiccup

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I think this naming makes sense.

Travis has had some reliability issues recently with reporting back so that's probably it, but manually clicking through shows it did indeed pass.

@ghoneycutt I'll leave this for you since you previously had comments.

@bastelfreak bastelfreak merged commit 8ba502f into voxpupuli:master Apr 22, 2020
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.

None yet

4 participants