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.

Sign up for GitHub

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

Jump to bottom

add 'repo' install method to fetch and install package directly from packagecloud #34

Merged
merged 10 commits into from
Jun 12, 2015
Merged

add 'repo' install method to fetch and install package directly from packagecloud #34

merged 10 commits into from
Jun 12, 2015

Conversation

rplessl
Copy link
Contributor

@rplessl rplessl commented Jun 4, 2015

This add's an additional install method 'repo' which includes the packagecloud repositories for install and update grafana. Enhanced for Debian/Ubuntu and RedHat.

Also fix package name of libfontconfig to libfontconfig1 which is the correct one on Ubuntu LTS. This fixes the reinstallation of libfontconfig each time the puppet runs.

spec file is enhanced, but needs a review from a more experienced spec file writer.

Roman Plessl added 5 commits June 3, 2015 13:20
added 'repo' method to install grafana from packageclound repository
32c8f23 
and obsolete 'wget' and 'archive' module with that way
fix key
c7216d9
this must be libfontconfig1 to fix
6c56e98 
  Notice: /Stage[main]/Grafana::Install/Package[libfontconfig]/ensure: ensure changed 'purged' to 'present'
changed test also to libfontconfig1
55b45e9
added more testing environment stuff
8ff54fd
@rplessl rplessl mentioned this pull request Jun 4, 2015
Closed
@adamcstephens
Copy link
Contributor

Many modules add an enable_repo parameter. This allows people who create mirrors or packages to opt out of the module managing the repo.

Otherwise, this looks great.

@bfraser
Copy link
Owner

bfraser commented Jun 5, 2015

Hi @rplessl thank you for the pull request! I'd like to merge this, though I have some comments / questions.

  1. I am in agreement with @adamcstephens that we should provide the ability for the module to not manage the package repository, in case the user has their own local repository they are maintaining.
  2. Have you tested this code on Red Hat systems? I ask because I don't believe this code will work as the package name is fontconfig.
  3. There is no ordering of the package resources via the require metaparameter, so there is the possibility of the module attempting to install Grafana before libfontconfig1 (or fontconfig, as the case may be).
  4. Items 2 and 3 are not caught by the tests in the repo install method context because the install_method parameter is not being set to repo and it is falling back to the default, package. See the code in the archive install method context for an example of this.

Again, thanks for the submission!

@rplessl
Copy link
Contributor Author

rplessl commented Jun 7, 2015

Hi @bfraser!

Thanks for your quick reply.

  1. I agree, there should be a way for local repositories ... I will change my code
  2. Not tested with RedHat System, I agree, the marked install section should be moved to plattform specific part ... will follow in the next commit.
  3. Agree, will fix that in the next commit.
  4. Ok, will fix that too.

Thanks for the review! A new submission of the pull request will follow.

@rplessl
Copy link
Contributor Author

rplessl commented Jun 8, 2015

Hi @bfraser @adamcstephens,

I have added the suggested points:

Finding 1 / 4: There exist now a configuration value manage_package_repo to enable / disable the packagecloud repository.

Finding 2 / 3: Fixed.

Thanks for review again!

@bfraser
Copy link
Owner

bfraser commented Jun 12, 2015

Hi @rplessl this looks good. Thank you, I appreciate the contribution!

bfraser added a commit that referenced this pull request Jun 12, 2015
@bfraser
Merge pull request #34 from rplessl/master
9f25331 
add 'repo' install method to fetch and install package directly from packagecloud
@bfraser bfraser merged commit 9f25331 into bfraser:master Jun 12, 2015
@bfraser bfraser mentioned this pull request Jun 12, 2015
Closed
bastelfreak pushed a commit to bastelfreak/puppet-grafana that referenced this pull request Apr 29, 2017
@alexjfisher
Merge pull request bfraser#34 from bastelfreak/fix_archlinux
c016479 
implement package_ensure param for archlinux
cegeka-jenkins pushed a commit to cegeka/puppet-grafana that referenced this pull request Oct 23, 2017
@rplessl
added switch case for prependency for fontconfig / libfontconfig1
dcad4b5 
fixes finding bfraser#2 of bfraser#34
cegeka-jenkins pushed a commit to cegeka/puppet-grafana that referenced this pull request Oct 23, 2017
@rplessl
reorder code parts and add depencies / fixes bfraser#2 and bfraser#3 of
70a92de 
bfraser#34
cegeka-jenkins pushed a commit to cegeka/puppet-grafana that referenced this pull request Oct 23, 2017
@rplessl
added configuration parameter to active / not activate the repository…
2eadeb8 
… management of the official grafana repo

fix testing rules for 'repo'
this fixes bfraser#1 / bfraser#4 of bfraser#34
cegeka-jenkins pushed a commit to cegeka/puppet-grafana that referenced this pull request Feb 1, 2024
@alexjfisher
Merge pull request bfraser#34 from bastelfreak/fix_archlinux
fac7939 
implement package_ensure param for archlinux
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rplessl @adamcstephens @bfraser @queeno