-
Notifications
You must be signed in to change notification settings - Fork 98
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
Rewrite to a shared GitHub Actions workflow #677
Conversation
@ekohl any plans to move this forward? |
221e3eb
to
60eda25
Compare
Plans would be a big word. There certainly is a desire to move it forward, but right now I'm working hard to avoid picking up new work so I can focus on the upcoming conferences and branching. I have transferred the repo to https://github.com/theforeman/actions a while back and rebased the PR to reflect that. theforeman/actions#1 has the notes on what should be done to move forward. I now see that dropping Ruby 2.5 from the tests is something that should be added to the list. If anyone wants to pick it up and take it over the finish line I'd be more than happy. |
60eda25
to
0160c9b
Compare
ed3b35f
to
cba7d9e
Compare
lib/foreman_tasks/tasks/test.rake
Outdated
Rake::TestTask.new(foreman_tasks: 'db:test:prepare') do |t| | ||
t.libs << ForemanTasks::Engine.root.join('test') | ||
t.pattern = ForemanTasks::Engine.root.join('test/**/*_test.rb') | ||
t.test_files = [Rails.root.join('test/unit/foreman/access_permissions_test.rb')] |
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.
In OpenSCAP I saw a dedicated task that ran the tests from core where it modified things. Especially when some controllers are extended I think that's a good pattern to keep, and it may help with setting up correct load paths. I'm now debating that approach as well.
lib/foreman_tasks/tasks/test.rake
Outdated
desc 'Foreman Tasks plugin tests' | ||
Rake::TestTask.new(foreman_tasks: 'db:test:prepare') do |t| |
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.
One downside of this is that it'll also be included in foreman-rake
and executed, which slows things down. The previous approach only defined a very simple task and loaded things on demand.
We may want to exclude this file from the gem or RPMs/debs.
This differs a little bit from what is suggested in https://github.com/theforeman/actions#foreman-plugin-tests |
cba7d9e
to
02ca0a7
Compare
foreman-tasks is special, because of the plugin name ( I've rebased this and made some small tweaks to make the differences to the reference a bit smaller. |
lib/foreman_tasks/tasks/test.rake
Outdated
t.libs << Rails.root.join('test') | ||
t.libs << ForemanTasks::Engine.root.join('test') | ||
t.pattern = ForemanTasks::Engine.root.join('test', '**', '*_test.rb') | ||
t.test_files = [Rails.root.join('test', 'unit', 'foreman', 'access_permissions_test.rb')] |
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.
Should this be dropped so that we're consistent with how the test task is configured in other plugins?
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.
If you prefer, I can drop it and then we can solve it consistently with all plugins.
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 do, unless we'll fix it everywhere this way
This alias matches the plugin name, which makes it easier to write a reusable action.
These reusable actions remove a lot of duplication. It also respects the matrix configuration provided in Foreman itself, meaning it will automatically detect the supported Ruby and NodeJS versions.
02ca0a7
to
babbc1d
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.
APT
Thank you @ekohl ! |
This is an effort to align all plugin testing on reusable workflows. For now this is mostly testing if I've missed something, so I'm going over the various plugins.