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.

Sign up for GitHub

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

Jump to bottom

Avoid Type=notify for squid service in github CI #170

Merged
merged 2 commits into from
Mar 24, 2022
Merged

Avoid Type=notify for squid service in github CI #170

merged 2 commits into from
Mar 24, 2022

Conversation

traylenator
Copy link
Contributor

@traylenator traylenator commented Mar 22, 2022 •
edited
Loading

Pull Request (PR) description

squid is failing to start on CentOS 8 acceptance tests with a timeout.

Avoid Type=notify for squid service as it seems the notification never gets there within Gitlab CI even if the service does start.

@traylenator traylenator marked this pull request as draft March 22, 2022 16:05
@traylenator
Avoid Type=notify for squid service
16232ef 
squid was timing out on startup on CentOS 8
@traylenator traylenator changed the title Cat squids cache.log so we can debug Avoid Type=notify for squid service in github CI Mar 22, 2022
@traylenator traylenator marked this pull request as ready for review March 22, 2022 19:00
This was referenced Mar 22, 2022
Merged
Merged
smortex
smortex approved these changes Mar 23, 2022
View reviewed changes
Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

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

Ugh, I don't like this but unfortunately have nothing better to propose and it fix CI so… LGTM?

@traylenator
Copy link
Contributor Author

Ugh, I don't like this but unfortunately have nothing better to propose and it fix CI so… LGTM?

Indeed.

Couple of extra things that could be done:

  • Is there a suitable fact so we could use to do this only in gitlab CI. virtual=kvm does not seem great.
  • Could add an additional test without the extra drop file where we expect it to fail. This would catch if the situation is fixed in the future. It's not obvious why this broke. Previous squid package on C8 also used notify.

@traylenator
Copy link
Contributor Author

pending test added, that will also catch if Type=notify turns out to work on CentOS 9 when it is added.

@traylenator
Copy link
Contributor Author

@smortex okay with the extra addition?

@smortex
Copy link
Member

smortex commented Mar 23, 2022

I am fine with this :-)

@traylenator traylenator merged commit 9d4e083 into voxpupuli:master Mar 24, 2022
@traylenator traylenator deleted the eight branch March 24, 2022 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smortex smortex smortex approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@traylenator @smortex