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

Fixes #37095 - Use shared GitHub workflow to run tests #10780

Merged
merged 2 commits into from
Feb 2, 2024
Merged

Fixes #37095 - Use shared GitHub workflow to run tests #10780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27f1961
Fixes #37095 - Use shared GitHub workflow to run tests
evgeni Oct 27, 2023
5cf67ba
Refs #37095 - move test dependencies into an own group
evgeni Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .github/workflows/ruby.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
---
name: CI

on:
pull_request:
push:
branches:
- 'master'
- 'KATELLO-*'

concurrency:
group: ${{ github.ref_name }}-${{ github.workflow }}
cancel-in-progress: true

jobs:
rubocop:
name: Rubocop
uses: theforeman/actions/.github/workflows/rubocop.yml@v0

test:
name: Ruby
needs: rubocop
uses: theforeman/actions/.github/workflows/foreman_plugin.yml@v0
with:
plugin: katello
postgresql_container: ghcr.io/theforeman/postgresql-evr
test_existing_database: false
Comment on lines +23 to +27
Copy link
Member Author

Choose a reason for hiding this comment

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

One thing to consider for branching. When Katello gets branched, you'll have to add a foreman_version: X.Y-stable here, as this is what's driving which Foreman branch is used for testing, and you want them to match ;-)

Copy link
Member

Choose a reason for hiding this comment

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

Can that be handled somehow centrally through some mapping or matrix file?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no such thing today (which of course doesn't mean we can't invent it…)

Today, this mapping lives in https://github.com/theforeman/jenkins-jobs/blob/master/theforeman.org/pipelines/test/testKatello.groovy#L1 and needs to be edited by whoever does the branching. So it's not a workflow change as such.

Copy link
Member

Choose a reason for hiding this comment

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

@wbclark @qcjames53 assuming this merges as-is, we'll need a new branching step to update the foreman_version in this ruby.yml file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think to make it more explicit, we could go and set foreman_version: develop today, so it can become s/develop/3.9-stable/ instead of adding a new line?

4 changes: 4 additions & 0 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
source 'https://rubygems.org'

gemspec

Dir[File.join(__dir__, 'gemfile.d', '*.rb')].each do |bundle|
eval_gemfile(bundle)
Copy link
Member

Choose a reason for hiding this comment

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

It is a shame dependabot doesn't understand this, where it did understand the previous format.

Copy link
Member Author

Choose a reason for hiding this comment

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

in this very particular case, we could just write eval_gemfile('gemfile.d/test.rb') and Dependabot would work, but otherwise I agree.

end
4 changes: 4 additions & 0 deletions gemfile.d/test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
group :test do
Copy link
Member

Choose a reason for hiding this comment

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

Why move these into this group in a Gemfile rather than keep them where they are?

Copy link
Member Author

Choose a reason for hiding this comment

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

Today, they are defined as development dependencies, but we do not install the "development" group in CI as that would be silly (we don't need pry and friends from https://github.com/theforeman/foreman/blob/develop/bundler.d/development.rb).
By moving them to a bundler.d file, we can define them as a group (you can't do that in the gemspec) and install cleanly.

Copy link
Member Author

Choose a reason for hiding this comment

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

More in my old comment at #10780 (review)

Copy link
Member

Choose a reason for hiding this comment

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

I agree and disagree. These testing frameworks are, I would argue, part of development. We shouldn't divorce testing from development.

Copy link
Member Author

Choose a reason for hiding this comment

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

But are we divorcing?
bundle install still installs them by default, we just can avoid pure dev deps (pry) by using "without development" config if we want to (in CI).
And this follows the same pattern we have in core.

Copy link
Member

Choose a reason for hiding this comment

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

Technically development dependencies in a gemspe end up in a bundler group called development. This just further split it.

It should be noted you can no longer load them via gemspec in a bundler file. Perhaps that's something that will show up in gems that depend on katello and want to use its tests

Copy link
Member Author

Choose a reason for hiding this comment

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

I've opened theforeman/jenkins-jobs#410 to at least fix Jenkins support for this setup.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ehelms do the recent change to move all devel-like deps to gemfile.d/test.rb satisfy your need for "consistency"?

Copy link
Member

Choose a reason for hiding this comment

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

I sense snark in the force. And yes it does. Thank you.

gem "vcr", "~> 6.1"
gem 'theforeman-rubocop', '~> 0.0.6', require: false
end
10 changes: 0 additions & 10 deletions katello.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,4 @@ Gem::Specification.new do |gem|
# UI
gem.add_dependency "deface", '>= 1.0.2', '< 2.0.0'
gem.add_dependency "angular-rails-templates", "~> 1.1.0"

# Testing
gem.add_development_dependency "factory_bot_rails", "~> 4.5"
gem.add_development_dependency "mocha"
gem.add_development_dependency "vcr", "~> 6.1"
gem.add_development_dependency "webmock"
gem.add_development_dependency "robottelo_reporter"

# Rubocop
gem.add_development_dependency 'theforeman-rubocop', '~> 0.0.6'
end
Loading