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

use voxpupuli/archive to download composer #274

Merged
merged 1 commit into from
Dec 1, 2016
Merged

use voxpupuli/archive to download composer #274

merged 1 commit into from
Dec 1, 2016

Conversation

igalic
Copy link
Contributor

@igalic igalic commented Nov 29, 2016

rather than using a hand-crafted curl call, we use voxpupuli/archive to
download composer. This has the benefit that it doesn't require us to
$manage_curl. But we also remove $environment! This variable name is
poorly chosen. Overriding it (or even just setting it to undef), could
mean that someone's hiera() lookups are failing, as soon as they enter
our module!

This pull-request addresses #273.

creates => $path,
proxy_type => $proxy_type,
proxy_server => $proxy_server,
require => [Class['::php::cli'],Package['curl']],
Copy link
Member

Choose a reason for hiding this comment

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

require Package['curl'] not needed now

) {

if $caller_module_name != $module_name {
warning('php::composer::auto_update is private')
}

$env = $proxy_type? {
undef => {},
Copy link
Member

Choose a reason for hiding this comment

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

It appears this isn't puppet 3 compatible. From https://docs.puppet.com/puppet/3.8/reference/lang_conditional.html#values

Values may be any of the following:

Any literal value, with the exception of hash literals

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'll have to return undef then.

creates => $path,
proxy_type => $proxy_type,
proxy_server => $proxy_server,
require => Class['::php::cli'],
Copy link
Member

Choose a reason for hiding this comment

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

This dependency looks a bit odd. But it was there before, so maybe removing it will have unintended consequences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

composer requires php to run, so this makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

to run, but not to download. Maybe it should just be included instead?

rather than using a hand-crafted curl call, we use voxpupuli/archive to
download composer. This has the benefit that it doesn't require us to
$manage_curl. But we also remove `$environment`! This variable name is
poorly chosen. Overriding it (or even just setting it to `undef`), could
mean that someone's `hiera()` lookups are failing, as soon as they enter
our module!

This pull-request addresses #273.
@edestecd
Copy link
Contributor

$environment is not needed with archive, because archive has specific proxy settings. Just make sure you pass them through so we can set them, when you make the change.

@igalic
Copy link
Contributor Author

igalic commented Nov 30, 2016

@edestecd
that's why i removed environment, however, as i hint at in voxpupuli/puppet-archive#244, there's room for improval in archive ;)

@alexjfisher alexjfisher merged commit 5ad2323 into voxpupuli:master Dec 1, 2016
@igalic igalic deleted the use-archive branch December 1, 2016 10:16
@igalic
Copy link
Contributor Author

igalic commented Dec 1, 2016

Error: Parameter path failed on Archive[download composer]: archive path must be absolute: download composer at /etc/puppetlabs/code/environments/production/modules/php/manifests/composer.pp:45

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.

3 participants