Ship our local version of google-chrome and chromedriver #1080
Conversation
Using the chromedriver branch I were able to run 'make tests_all' in a smoothly way. |
@@ -0,0 +1,36 @@ | |||
# install chromedriver for functional tests | |||
# we ship our local copy of chromedriver | |||
# because latest versions aren't working very well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to be more specific here (i.e. point to a github issue or explain why)
class chromedriver { | ||
|
||
file { '/tmp/google-chrome-stable_54.0.2840.100-1_amd64.deb': | ||
source => 'puppet:///modules/chromedriver/google-chrome-stable_54.0.2840.100-1_amd64.deb', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't put big binaries into git. We decided to not do this wuth the useragent debs and rather put them at github releases for downloading. Could you do the same for chrome/-driver pls ?
see https://github.com/pixelated/puppet-pixelated/blob/master/manifests/install.pp
mode => '0755', | ||
} | ||
|
||
exec { 'install_google_chrome': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of an exec you can use the Package resource like in https://github.com/pixelated/puppet-pixelated/blob/master/manifests/install.pp#L22
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure we can force a /usr/bin/apt-get -y -f install
with this resource?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you're right, I oversaw that one. I don't know if this is possible, so we can stick to an exec here.
To don't run the exec every time pls add sth like this:
unless => '/usr/bin/dpkg -l chromedriver > /dev/null 2>&1'
] | ||
} | ||
|
||
exec { 'unpack_chromedriver': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without a creates file attribute this exec would be called every time the provisioning is run. Please add a creates
to avoid this.
The latest chromedriver version is getting recurring errors when running login.feature from functional tests
dd0ec65
to
b946730
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great besides my two comments. I'm just testing it and if it works, pls just add the two requires, test it and merge it !
exec { 'unpack_chromedriver': | ||
command => "/usr/bin/unzip ${chromedriver} -d /usr/local/bin/", | ||
cwd => '/var/tmp/', | ||
creates => '/usr/local/bin/chromedriver', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This resource depends on Exec['fetch_chromedriver'] so pls add a require
here.
command => "/usr/bin/dpkg -i ${google_chrome} || /usr/bin/apt-get -y -f install", | ||
cwd => '/var/tmp/', | ||
unless => '/usr/bin/dpkg -l google-chrome-stable > /dev/null 2>&1', | ||
require => [ Exec['apt_get_update'] ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This resource depends on Exec['fetch_google_chrome'] so pls add it to the require
array here.
Fix functional test step
Then I should see the fancy interstitial
, that's not working properly with newer versions of chromedriver.Since the debian repo doesn't provide old versions of chromium, we're shipping our own local copies: