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

evgeni · 2023-10-27T06:31:40Z

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

ekohl · 2023-11-01T14:01:35Z

Looks like RuboCop isn't in the dependencies. I assume previously it ran from the Foreman codebase. On the DB side we're missing the EVR extension.

evgeni · 2023-11-28T08:49:09Z

katello.gemspec

+  gem.add_dependency "simplecov"
+  gem.add_dependency "simplecov-rcov"


That's a terrible hack, and I wonder how to "correctly" fix this.

add_development_dependency adds things to the development group of Bundler, but we do not install this group as we only want to run tests. There is no add_test_dependency.

I see three possible solutions, and not quite sure which one I like most:

install development group (and pull in all those really dev-only deps of foreman, meh)

set development_group to something else (like ${plugin}_dev) when including the plugin in our actions, this would install all plugin dev deps as the new group is not excluded

provide a gemfile.d/test.rb that clearly defines the test deps in the test group.

I think 3 is the cleanest, but will also require the most changes (to every plugin that needs to define test deps)

2 is what Jenkins does today, btw:
https://github.com/theforeman/jenkins-jobs/blob/6dd3bfe1fa010ecbb4750d9e909246a148c410fb/theforeman.org/pipelines/lib/foreman.groovy#L7

katello.gemspec

gemfile.d/test.rb

ekohl · 2023-11-28T10:44:51Z

Gemfile

@@ -1,3 +1,7 @@
 source 'https://rubygems.org'

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


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

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

.github/workflows/ruby.yml

gemfile.d/test.rb

.github/workflows/ruby.yml

ehelms · 2024-01-04T19:21:11Z

gemfile.d/test.rb

@@ -0,0 +1,7 @@
+group :test do


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

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.

More in my old comment at #10780 (review)

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

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.

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

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

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

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

db/migrate/20160404132250_remove_katello_from_notification_name.rb

ianballou · 2024-01-10T18:49:45Z

When a redmine gets created for this, mind linking this redmine to the new one? https://projects.theforeman.org/issues/36998

.github/workflows/ruby.yml

db/migrate/20160505181337_rename_katello_settings.rb

db/migrate/20131014225132_add_users_fields.rb

ekohl · 2024-01-23T11:27:50Z

Because the issue is getting quite long, a list of split off PRs:

evgeni · 2024-01-23T14:09:21Z

Because the issue is getting quite long, a list of split off PRs:

FWIW, #10852, #10853 and #10854 are not explicitly required for the shared action work. They are a path forward to make Katello installable ontop of an existing Foreman, which is e.g. required to test that plugins that require Katello are installable after the fact, but today we just disable this part via test_existing_database: false.

evgeni · 2024-01-24T07:36:26Z

katello.gemspec

@@ -64,18 +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"
-  gem.add_development_dependency "uglifier"


evgeni · 2024-01-24T08:49:45Z

This is now 🍏 and ready!

evgeni · 2024-01-25T07:37:14Z

katello.gemspec

-  gem.add_development_dependency "factory_bot_rails", "~> 4.5"
-  gem.add_development_dependency "mocha"
-  gem.add_development_dependency "vcr", "< 4.0.0"
-  gem.add_development_dependency "webmock"
-  gem.add_development_dependency "robottelo_reporter"


I've dropped factory_bot_rails, moch, webmock and robottelo_reporter as they are all in https://github.com/theforeman/foreman/blob/develop/bundler.d/test.rb

Only vcr is Katello-specific in that list.

evgeni · 2024-01-25T08:21:45Z

.github/workflows/ruby.yml

+    uses: theforeman/actions/.github/workflows/foreman_plugin.yml@v0
+    with:
+      plugin: katello
+      postgresql_container: ghcr.io/theforeman/postgresql-evr
+      test_existing_database: false


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 ;-)

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

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.

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

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?

Also drop some unused dependencies

ianballou

Does this require a matching Foreman PR to work? I'm hitting issues loading vcr now when running tests locally:

vagrant@centos8-katello-devel-stable ~/katello $ ktest test/services/katello/pulp3/rpm_test.rb 
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activerecord-6.1.7.6/lib/active_record/connection_adapters/postgresql_adapter.rb:883: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/pg-1.5.4/lib/pg/text_decoder/timestamp.rb:8: warning: The called method `initialize' is defined here
Emptying /home/vagrant/foreman/jenkins/reports/unit
-- execute("SET CONSTRAINTS ALL DEFERRED;")
WARNING:  SET CONSTRAINTS can only be used in transaction blocks
   -> 0.0003s
Traceback (most recent call last):
	19: from -e:1:in `<main>'
	18: from -e:1:in `each'
	17: from -e:1:in `block in <main>'
	16: from -e:1:in `require'
	15: from /home/vagrant/katello/test/services/katello/pulp3/rpm_test.rb:1:in `<top (required)>'
	14: from /home/vagrant/katello/test/services/katello/pulp3/rpm_test.rb:1:in `require'
	13: from /home/vagrant/katello/test/katello_test_helper.rb:12:in `<top (required)>'
	12: from /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activesupport-6.1.7.6/lib/active_support/dependencies.rb:332:in `require'
	11: from /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activesupport-6.1.7.6/lib/active_support/dependencies.rb:299:in `load_dependency'
	10: from /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activesupport-6.1.7.6/lib/active_support/dependencies.rb:332:in `block in require'
	 9: from /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/zeitwerk-2.6.12/lib/zeitwerk/kernel.rb:38:in `require'
	 8: from /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/polyglot-0.3.5/lib/polyglot.rb:65:in `require'
	 7: from /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/polyglot-0.3.5/lib/polyglot.rb:65:in `require'
	 6: from /home/vagrant/katello/test/support/vcr.rb:1:in `<top (required)>'
	 5: from /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activesupport-6.1.7.6/lib/active_support/dependencies.rb:332:in `require'
	 4: from /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activesupport-6.1.7.6/lib/active_support/dependencies.rb:299:in `load_dependency'
	 3: from /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/activesupport-6.1.7.6/lib/active_support/dependencies.rb:332:in `block in require'
	 2: from /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/zeitwerk-2.6.12/lib/zeitwerk/kernel.rb:38:in `require'
	 1: from /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/polyglot-0.3.5/lib/polyglot.rb:65:in `require'
/home/vagrant/foreman/.vendor/ruby/2.7.0/gems/polyglot-0.3.5/lib/polyglot.rb:65:in `require': cannot load such file -- vcr (LoadError)

evgeni · 2024-01-30T19:00:20Z

Does this require a matching Foreman PR to work? I'm hitting issues loading vcr now when running tests locally:

Ah, this is a good point.
You (or whatever sets up the devel box) would need to know about gemfile.d too.

https://github.com/theforeman/actions/blob/v0/.github/workflows/foreman_plugin.yml#L103

a cat like this could do (or a symlink, then changes to the file would propagate automatically)

ianballou · 2024-01-30T19:36:14Z

a cat like this could do (or a symlink, then changes to the file would propagate automatically)

Sounds puppet-katello_devel should handle this https://github.com/theforeman/puppet-katello_devel

ianballou

I'm cool with merging this in -- the necessary additions to katello.gemspec.local are likely to cause a bit of confusion among katello devs, but I suppose it doesn't make sense to wait for a new puppet-katello_devel PR first.

* Fixes #37095 - Use shared GitHub workflow to run tests * Refs #37095 - move test dependencies into an own group Also drop some unused dependencies (cherry picked from commit 16be014)

pr-processor bot added Not yet reviewed Waiting on contributor labels Oct 27, 2023

evgeni force-pushed the gha-plugin-test branch from c001702 to f41459b Compare October 27, 2023 06:37

evgeni force-pushed the gha-plugin-test branch from f41459b to cd71d49 Compare November 28, 2023 07:59

github-actions bot added the Packaging Change label Nov 28, 2023

evgeni force-pushed the gha-plugin-test branch from 488c9d1 to 69f198e Compare November 28, 2023 08:19

evgeni commented Nov 28, 2023

View reviewed changes

evgeni force-pushed the gha-plugin-test branch 3 times, most recently from de136b5 to 74b48d8 Compare November 28, 2023 10:27

ekohl reviewed Nov 28, 2023

View reviewed changes

ekohl suggested changes Dec 27, 2023

View reviewed changes

.github/workflows/ruby.yml Outdated Show resolved Hide resolved

pr-processor bot removed the Not yet reviewed label Dec 27, 2023

evgeni force-pushed the gha-plugin-test branch from 74b48d8 to a32f1d9 Compare January 2, 2024 08:21

pr-processor bot added the Needs re-review label Jan 2, 2024

evgeni force-pushed the gha-plugin-test branch from 24f1b2c to 8c6adec Compare January 2, 2024 08:45

ekohl reviewed Jan 2, 2024

View reviewed changes

gemfile.d/test.rb Outdated Show resolved Hide resolved

gemfile.d/test.rb Outdated Show resolved Hide resolved

ekohl reviewed Jan 4, 2024

View reviewed changes

.github/workflows/ruby.yml Outdated Show resolved Hide resolved

ehelms reviewed Jan 4, 2024

View reviewed changes

ekohl reviewed Jan 5, 2024

View reviewed changes

db/migrate/20160404132250_remove_katello_from_notification_name.rb Outdated Show resolved Hide resolved

evgeni force-pushed the gha-plugin-test branch 2 times, most recently from 94f345d to c37ba65 Compare January 9, 2024 14:53

ekohl mentioned this pull request Jan 18, 2024

Verify it works with all plugins theforeman/actions#1

Closed

29 tasks

ekohl reviewed Jan 19, 2024

View reviewed changes

.github/workflows/ruby.yml Outdated Show resolved Hide resolved

evgeni force-pushed the gha-plugin-test branch from c37ba65 to 97fc34d Compare January 22, 2024 07:00

evgeni commented Jan 22, 2024

View reviewed changes

db/migrate/20160505181337_rename_katello_settings.rb Outdated Show resolved Hide resolved

evgeni commented Jan 22, 2024

View reviewed changes

db/migrate/20131014225132_add_users_fields.rb Outdated Show resolved Hide resolved

evgeni force-pushed the gha-plugin-test branch from 97fc34d to 4e80072 Compare January 23, 2024 07:33

evgeni force-pushed the gha-plugin-test branch from 5dc9513 to 35acc4b Compare January 24, 2024 06:49

evgeni commented Jan 24, 2024

View reviewed changes

evgeni force-pushed the gha-plugin-test branch from 822f3e5 to be5139a Compare January 24, 2024 07:44

pr-processor bot removed the Waiting on contributor label Jan 24, 2024

evgeni changed the title ~~use shared github workflow to run tests~~ Fixes #37095 - Use shared GitHub workflow to run tests Jan 24, 2024

evgeni marked this pull request as ready for review January 24, 2024 08:14

evgeni closed this Jan 24, 2024

evgeni reopened this Jan 24, 2024

evgeni force-pushed the gha-plugin-test branch from be5139a to f18f27b Compare January 25, 2024 06:46

evgeni commented Jan 25, 2024

View reviewed changes

evgeni force-pushed the gha-plugin-test branch from f18f27b to d4c9455 Compare January 25, 2024 13:49

ofedoren mentioned this pull request Jan 25, 2024

Fixes #37145 - Ruby 3 support #10861

Closed

evgeni added 2 commits January 25, 2024 15:41

Fixes #37095 - Use shared GitHub workflow to run tests

27f1961

Refs #37095 - move test dependencies into an own group

5cf67ba

Also drop some unused dependencies

evgeni force-pushed the gha-plugin-test branch from d4c9455 to 5cf67ba Compare January 25, 2024 14:41

ianballou reviewed Jan 30, 2024

View reviewed changes

ianballou approved these changes Jan 30, 2024

View reviewed changes

pr-processor bot removed the Needs re-review label Jan 30, 2024

evgeni mentioned this pull request Feb 1, 2024

Rewrite workflow using reusable actions theforeman/foreman_monitoring#95

Merged

jeremylenz merged commit 16be014 into Katello:master Feb 2, 2024
11 of 12 checks passed

evgeni mentioned this pull request Apr 19, 2024

Use 3.10-stable for GHA based tests, not develop #10968

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

evgeni commented Oct 27, 2023

ekohl commented Nov 1, 2023

evgeni Nov 28, 2023

evgeni Nov 28, 2023

ekohl Nov 28, 2023

evgeni Nov 28, 2023

ehelms Jan 4, 2024

evgeni Jan 4, 2024

evgeni Jan 4, 2024

ehelms Jan 8, 2024

evgeni Jan 8, 2024

ekohl Jan 8, 2024

evgeni Jan 23, 2024

evgeni Jan 24, 2024

ehelms Jan 25, 2024

ianballou commented Jan 10, 2024

ekohl commented Jan 23, 2024

evgeni commented Jan 23, 2024

evgeni Jan 24, 2024

evgeni commented Jan 24, 2024

evgeni Jan 25, 2024

evgeni Jan 25, 2024

ehelms Jan 25, 2024

evgeni Jan 25, 2024

ianballou Jan 30, 2024

evgeni Jan 30, 2024

ianballou left a comment

evgeni commented Jan 30, 2024

ianballou commented Jan 30, 2024

ianballou left a comment

		gem.add_dependency "simplecov"
		gem.add_dependency "simplecov-rcov"

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

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

Conversation

evgeni commented Oct 27, 2023

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

ekohl commented Nov 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou commented Jan 10, 2024

ekohl commented Jan 23, 2024

evgeni commented Jan 23, 2024

Choose a reason for hiding this comment

evgeni commented Jan 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianballou left a comment

Choose a reason for hiding this comment

evgeni commented Jan 30, 2024

ianballou commented Jan 30, 2024

ianballou left a comment

Choose a reason for hiding this comment