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

Only install timesync packages if needed #1853

Merged
merged 1 commit into from
Aug 27, 2024
Merged

Only install timesync packages if needed #1853

merged 1 commit into from
Aug 27, 2024

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Mar 13, 2024

Chrony/NTP is only needed if timesync is requested. In some cases (like containers) it never makes sense.

This was inspired by voxpupuli/beaker-hostgenerator#359 (comment) and currently testing it out.

@ekohl ekohl mentioned this pull request Mar 13, 2024
Open
@codecov Codecov
Copy link

codecov bot commented Mar 13, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 13.33333% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 51.60%. Comparing base (2a89cfc) to head (e3ec2d5).

❗ Current head e3ec2d5 differs from pull request most recent head c2d1b84. Consider uploading reports for the commit c2d1b84 to get more accurate results

Files Patch % Lines
lib/beaker/platform.rb 11.53% 23 Missing ⚠️
lib/beaker/host_prebuilt_steps.rb 25.00% 3 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           master    #1853       +/-   ##
===========================================
- Coverage   74.62%   51.60%   -23.03%     
===========================================
  Files          81       81               
  Lines        4492     4498        +6     
===========================================
- Hits         3352     2321     -1031     
- Misses       1140     2177     +1037

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ekohl ekohl force-pushed the only-install-timesync-packages-if-needed branch from 37a4799 to 423fb62 Compare March 13, 2024 19:42
@ekohl ekohl force-pushed the only-install-timesync-packages-if-needed branch from 6700df9 to e3ec2d5 Compare March 14, 2024 15:02
@ekohl
Copy link
Member Author

ekohl commented Mar 14, 2024

This is still a draft. The fail because the tests don't ensure platform is an instance of Beaker::Platform, unlike the real application. #1855 is a start for that.

@ekohl ekohl mentioned this pull request Jul 23, 2024
Merged
@ekohl ekohl force-pushed the only-install-timesync-packages-if-needed branch 3 times, most recently from 27c5b04 to 594063b Compare July 24, 2024 15:32
@ekohl ekohl force-pushed the only-install-timesync-packages-if-needed branch from 594063b to 6fcba17 Compare August 27, 2024 16:42
@ekohl ekohl marked this pull request as ready for review August 27, 2024 16:42
@ekohl ekohl force-pushed the only-install-timesync-packages-if-needed branch from 6fcba17 to fc29950 Compare August 27, 2024 16:48
@ekohl
Copy link
Member Author

ekohl commented Aug 27, 2024

Test changes split off to #1900.

@ekohl
Only install timesync packages if needed
18e6e22 
Chrony/NTP is only needed if timesync is requested. In some cases (like
containers) it never makes sense.

Which packages are installed is really platform specific logic, so it
should live in the platform class.
@ekohl ekohl force-pushed the only-install-timesync-packages-if-needed branch from fc29950 to 18e6e22 Compare August 27, 2024 17:34
ekohl
ekohl commented Aug 27, 2024
View reviewed changes
acceptance/tests/base/host/packages.rb
@@ -34,7 +11,7 @@ def get_host_pkg(host)

step '#check_for_package : can determine if a package is installed'
hosts.each do |host|
package = get_host_pkg(host)[0]
package = 'bash'
Copy link
Member Author

Choose a reason for hiding this comment

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

This probably isn't correct for everything, but the old approach was getting invalid.

Copy link
Member

Choose a reason for hiding this comment

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

at least for everything we test on, it should be fine.

@bastelfreak bastelfreak merged commit 0e788e0 into voxpupuli:master Aug 27, 2024
7 checks passed
@ekohl ekohl deleted the only-install-timesync-packages-if-needed branch August 27, 2024 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bastelfreak bastelfreak bastelfreak approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@ekohl @bastelfreak