-
Notifications
You must be signed in to change notification settings - Fork 83
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
Use foreman GH action workflow #923
Conversation
@evgeni do you know why this is failing? |
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.
It's failing on a few things:
- Ruby 3.2 fails because Rake 10 doesn't work with it:
Line 9 in 9a6ae05
gem 'rake', '~> 10.1.0' - Ruby 3.1 - 2.6 fail on
gem build --verbose --strict *.gemspec
. Some of those can be resolved by merging thegemspec
withGemfile
dependencies. - Ruby 2.5 - 2.4 fail because there is no
--strict
parameter togem build
in those versions. I'd add a minimum Ruby version of 2.7 in the gemspec to avoid testing on those old versions.
.github/workflows/unit-tests.yml
Outdated
name: Tests | ||
uses: theforeman/actions/.github/workflows/test-gem.yml@v0 | ||
with: | ||
command: bundle exec rake test |
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.
I'd ensure a newline here.
- name: Add hammer-cli-foreman to local gem file | ||
run: echo "gem 'hammer_cli_foreman', :git => 'https://github.com/theforeman/hammer-cli-foreman.git'" > Gemfile.local | ||
- name: Add hammer-cli to local gem file | ||
run: echo "gem 'hammer_cli', :git => 'https://github.com/theforeman/hammer-cli.git'" >> Gemfile.local |
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.
These are now only pulled in via released gems, no longer the git version. If you care about this, I'd recommend adding it to your Gemfile. If you want, you can limit it to ENV['GITHUB_ACTIONS'] == 'true'
.
|
||
gem.add_development_dependency "rubocop", "0.42" | ||
gem.add_development_dependency "rubocop-checkstyle_formatter" | ||
gem.add_development_dependency 'rubocop', '0.42' |
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.
This is a seriously old version. It wouldn't surprise me if this wouldn't work on Ruby 3.
hammer_cli_katello.gemspec
Outdated
gem.add_development_dependency "rubocop-checkstyle_formatter" | ||
gem.add_development_dependency 'rubocop', '0.42' | ||
gem.add_development_dependency 'rubocop-checkstyle_formatter' | ||
# for generating i18n files, gettext > 3.0 dropped ruby 1.8 support |
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.
I think the Ruby 1.8 part isn't that relevant anymore.
gem.add_development_dependency "rubocop", "0.42" | ||
gem.add_development_dependency "rubocop-checkstyle_formatter" | ||
gem.add_development_dependency 'rubocop', '0.42' | ||
gem.add_development_dependency 'rubocop-checkstyle_formatter' |
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.
This was only used for testing in Jenkins, but even that doesn't need it anymore.
gem.add_development_dependency 'rubocop-checkstyle_formatter' |
# for generating i18n files, gettext > 3.0 dropped ruby 1.8 support | ||
gem 'gettext', '>= 3.1.3', '< 4.0.0' | ||
|
||
group :test do |
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.
We install RuboCop testing without development dependencies, but with test dependencies. So just moving everything to a development dependency broke the RuboCop.
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.
Ah so these all would need to be done as:
gem.add_dependency
then since I was looking at the docs for gemspec
and you can't do grouping like you could in the Gemfile
If that is correct, I will go ahead and get it changed up
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.
Yeah, in a gemspec you can only do add_dependency
(which is runtime) and add_development_dependency
which is devel and maps to the development
bundler group from the gemfile
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.
fwiw, @ekohl is wrong, we do not skip development
group when installing rubocop: https://github.com/theforeman/actions/blob/v0/.github/workflows/rubocop.yml
we do that only in the plugin flow: https://github.com/theforeman/actions/blob/v0/.github/workflows/foreman_plugin.yml#L45
|
||
gem.add_dependency 'hammer_cli_foreman' | ||
gem.add_dependency 'hammer_cli_foreman_tasks' | ||
gem.add_dependency 'gettext', '>= 3.1.3', '< 4.0.0' |
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.
this is a devel dependency I think?
#924 is turning green now, see the individual commits in that PR for an explanation why/what has happened there. |
Thanks for putting them into single commits, I see what I was doing wrong and what you meant now about my question about Ewoud's comment and then yours. This helps me do hammer azure/virt-who/rh_cloud so appreciate you doing this one as an example. Closing my pr out in favor of yours. |
No description provided.