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

modulesync 2.12.0 #513

Merged
merged 9 commits into from
Apr 11, 2020
Merged

modulesync 2.12.0 #513

merged 9 commits into from
Apr 11, 2020

Conversation

bastelfreak
Copy link
Member

modulesync 2.7.0

@c33s
Copy link
Member

c33s commented Apr 8, 2019

is it really a good step to remove puppet4 support? what are the benefits of dropping the support for puppet4?

debians current lts is supported until June 2022 (stretch) which ships puppet 4.8. you are defacto killing the native puppet support in currents debian stable.

https://en.wikipedia.org/wiki/Debian_version_history#Debian_9_(Stretch)

@bastelfreak
Copy link
Member Author

Hi @c33s. This is the result of a pretty long discussion which happend in voxpupuli/voxpupuli.github.io#157.

@dhoppe dhoppe added the needs-work not ready to merge just yet label Apr 30, 2019
@dhoppe
Copy link
Member

dhoppe commented Apr 30, 2019

These tests must be converted from mocha to rspec-mocks.

@vox-pupuli-tasks
Copy link

Dear @bastelfreak, 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

@dhoppe dhoppe changed the title modulesync 2.7.0 and drop puppet 4 modulesync 2.9.0 and drop puppet 4 Dec 6, 2019
@vox-pupuli-tasks
Copy link

Dear @bastelfreak, 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

@dhoppe dhoppe changed the title modulesync 2.9.0 and drop puppet 4 modulesync 2.10.0 and drop puppet 4 Jan 22, 2020
@dhoppe dhoppe changed the title modulesync 2.10.0 and drop puppet 4 modulesync 2.10.1 and drop puppet 4 Jan 22, 2020
@bastelfreak bastelfreak changed the title modulesync 2.10.1 and drop puppet 4 modulesync 2.12.0 and drop puppet 4 Apr 4, 2020
@EmilienM
Copy link

EmilienM commented Apr 4, 2020

LGTM

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.

Either needs the tests rewritten to use rspec-mocks or be configured to use mocks. I have a preference to standardize on rspec-mocks but as long as it's green, I'm happy.

@@ -19,7 +19,4 @@
- set: debian9-64
options:
script: 'bundle exec rspec spec/acceptance/php_spec.rb'
- set: debian8-64
Copy link
Member

Choose a reason for hiding this comment

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

@bastelfreak We should keep this, since Debian 8 is not EOL until 2020-06-30.

@dhoppe dhoppe force-pushed the modulesync branch 3 times, most recently from 842dee2 to 3845a84 Compare April 8, 2020 09:55
@@ -10,12 +10,10 @@
end

case default[:platform]
when %r{ubuntu-18.04}
Copy link
Member

Choose a reason for hiding this comment

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

I'm generally a fan of smaller diffs and keeping it newest to oldest is OK IMHO. It's still sorted and consistent.

@dhoppe dhoppe force-pushed the modulesync branch 2 times, most recently from dfef06c to 060d46f Compare April 8, 2020 11:35
expect(described_class.instances).to eq []
end
end

describe '#install' do
it 'installs a package' do
described_class.expects(:pear).
allow(described_class).to receive(:pear).
with('-D', 'auto_discover=1', 'upgrade', '--alldeps', 'dummy')
provider.install
Copy link
Member

Choose a reason for hiding this comment

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

To really replace this and keep the test, you should add a have_received assertion. This is repeated so I won't comment it everywhere.

Suggested change
provider.install
provider.install
expect(described_class).to have_receive(:pear)

Copy link
Member

Choose a reason for hiding this comment

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

I already tried this, but it did not work as expected:

@bastelfreak bastelfreak removed the needs-work not ready to merge just yet label Apr 11, 2020
@bastelfreak
Copy link
Member Author

as this finally works, I'm going to merge it for now. We can still improve it later on.

@bastelfreak bastelfreak merged commit 06f2162 into master Apr 11, 2020
@bastelfreak bastelfreak deleted the modulesync branch April 11, 2020 14:03
@bastelfreak bastelfreak changed the title modulesync 2.12.0 and drop puppet 4 modulesync 2.12.0 Apr 11, 2020
@c33s
Copy link
Member

c33s commented Apr 11, 2020

yeah no puppet4 drop 👍 😄 💯 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants