Skip to content

added influxdb::source_url so you can pass this in instead of use ama…#1

Merged
rplessl merged 1 commit intorplessl:masterfrom
hurrycaine:source_url
Jul 29, 2015
Merged

added influxdb::source_url so you can pass this in instead of use ama…#1
rplessl merged 1 commit intorplessl:masterfrom
hurrycaine:source_url

Conversation

@hurrycaine
Copy link
Copy Markdown

…zon s3

This maintains the current functionality by default but adds a source_url that can be passed that overrides the amazon.s3 download url. It also expects full path, no url + name etc...

My companies servers do not have access to the internet and we store 3rd party apps in nexus.

I also updated the Readme and added unit tests to test the new conditional.

Please let me know if you have any questions!
I also have some other ideas...

@rplessl
Copy link
Copy Markdown
Owner

rplessl commented Jul 27, 2015

Thank you @hurrycaine for the pull request!

I'd like to merge this, though I have some remarks / comments / questions.

  1. Do you repackage and rename the package of InfluxDB or just copy the files to an internal server.

    Then the code here would have more consistency (by keeping package_source_name)

    If not repackaged / renamed I like to keep package_source_name in the install method.

  2. What's the reason for removing this test case?

  3. Thanks for also supplying the test cases! Really appreciate that 👍

Thanks again for your submission!

@rplessl
Copy link
Copy Markdown
Owner

rplessl commented Jul 27, 2015

download_url instead of source_url is in my point of view clearer

@hurrycaine
Copy link
Copy Markdown
Author

  1. I do not repackage and could use the source_package_name but I felt giving the full url either in hiera or in a module calling influxdb would have the most flexibility. I have a parametrized the url in my module that I pass to influxdb class. I do also wonder if they would ever have a '-2.x86_64' (I think unlikely). I use hiera for version etc...

Example:

class sensu_install::profiles::influxdb (
 ...
  $version       = '0.9.1',
  $classifier     = '1.x86_64',
)
...
url = "https://code.blah.com/artifacts/content/repositories/thirdparty/com/influxdb/influxdb/${version}/influxdb-${version}-${classifier}.rpm"

Do you agree with keeping it as the full url?

/2. Maybe I'm wrong but I thought it was a duplicate, under the context 'installing from a repository' there is already a check for package existing. It made less since to keep the package check in the top most describe scope after I added the context 'install from web'. It did not hurt to leave it in by any means, I can add it back.

/3. No problem. Thanks for all your hard work keeping this module up to date! It looks like the original owner is no longer keeping track of issue and pulls, and your fork seems to be the best fork. I wonder if at some time we should make this a non-fork, name it influxdb-puppet or something.

download_url vs source_url vs ?
Download_url is fine to me, I think I may have seen source_url used in other modules but it does not matter to me. Though in response to number 1 if we wanted to change how the download_url works, what would we call it download_root ?

@rplessl
Copy link
Copy Markdown
Owner

rplessl commented Jul 28, 2015

hi @hurrycaine!

  1. I see - makes sense for me too with hiera, use that myself for different version selectors on my servers ... also not sure myself if there will be never a minor update (like -2 for rpms / debs).

    I agree by keeping the full url (without using the source_package_name)

    download_url is fine for me in that case.

  2. After reading the test code the old / my enhanced package installed checking needs to be enhanced. Please remove the should contain package line 6. I will add new checking rules after merging your pull request.

  3. Thanks! Try my best to have a flexible puppet module for the influx config file and internal changes and also for the enhancements 😄

Please update the API point 1 ... I will merge it 👍 . Thx!

@hurrycaine
Copy link
Copy Markdown
Author

I have made the suggested updates, source_url is no more! (except the branch name...)

rplessl added a commit that referenced this pull request Jul 29, 2015
@rplessl rplessl merged commit 079a87a into rplessl:master Jul 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants