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

Add support for Debian 10 / Ubuntu 20.04 #135

Merged
merged 1 commit into from
Jan 10, 2021

Conversation

eheydrick
Copy link
Contributor

Description

The problem is cache initialization cannot run while squid is running. Older versions of squid silently failed but the version in Debian 10 / Ubuntu 20.04 returns an error. To solve this we stop squid before running cache init.

Also drop support for EOL OS's and add support for new ones.

Issues Resolved

Fixes #134 .

Check List

  • All tests pass. See TESTING.md for details.
  • New functionality includes testing.
  • New functionality has been documented in the README if applicable.

@eheydrick eheydrick requested a review from a team December 22, 2020 18:22
@@ -15,15 +15,16 @@ platforms:
- name: amazonlinux
driver_config:
box: mvbcoding/awslinux
- name: centos-6
- name: amazonlinux-2
- name: centos-7
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to shove centos 8 in here while you're in the file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converge is failing on Centos 8 - squid itself is starting and working but there's something to do with docker and systemd and squid being dumb that's causing the service start to timeout. It passes in vbox.

@@ -104,6 +104,8 @@
command "#{node['squid']['package']} -Nz"
action :run
creates ::File.join(node['squid']['cache_dir'], '00')
notifies :stop, "service[#{squid_service_name}]", :before
Copy link
Contributor

Choose a reason for hiding this comment

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

Before is pretty fragile and entirely broken in things like why-run. If there's a way to do it without this that'd be great

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other idea would be to drop initialization entirely since on at least debian the init script will create it if it doesn't exist. But I don't know if that's true across the board.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the upside, :before is doing the trick and all tests are passing and there is some precedence for using it within this codebase.

@@ -5,6 +5,12 @@
it { should be_listening }
end

cache_dir = os[:linux?] ? '/var/spool/squid' : '/var/squid/cache'
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you meant to have linux? inside the os hash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do os.linux? if that's more idiomatic. Both work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to os.linux? for better readability.

@eheydrick
Copy link
Contributor Author

Should I add a CHANGELOG entry? Didn't see an unreleased section in there. Maybe the bot hasn't got to it yet?

@xorima
Copy link
Contributor

xorima commented Dec 23, 2020

@eheydrick you are correct on the changelog, please add an entry under the heading ## Unreleased

We haven't done a release since the new system came in, so the bot won't have set things up properly yet.

Also drop testing on EOL OS's and add testing on Centos 8

Signed-off-by: Eric Heydrick <eheydrick@gmail.com>
@eheydrick
Copy link
Contributor Author

@xorima CHANGELOG entry added and also squashed.

@xorima xorima requested a review from tas50 December 24, 2020 09:57
@xorima xorima added the Release: Minor Release to Chef Supermarket as a minor release when merged label Dec 24, 2020
@eheydrick
Copy link
Contributor Author

Anything else needed here or ready to merge?

@xorima
Copy link
Contributor

xorima commented Jan 9, 2021

just need @tas50 to have a look over it :)

@tas50
Copy link
Contributor

tas50 commented Jan 9, 2021

I trust @eheydrick knows what he's doing. He's been reviewing my work for years

@xorima xorima merged commit bbe2cd7 into sous-chefs:master Jan 10, 2021
@kitchen-porter
Copy link
Contributor

Released as: 4.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release: Minor Release to Chef Supermarket as a minor release when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure on Ubuntu 20.04 / Debian 10
4 participants