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

Add GH actions CI testing #24

Closed
wants to merge 1 commit into from
Closed

Add GH actions CI testing #24

wants to merge 1 commit into from

Conversation

ezr-ondrej
Copy link
Member

@ezr-ondrej ezr-ondrej commented Dec 3, 2020 •
edited
Loading

Bumps the minimal Ruby version to match the smart-proxy version.
Adds the rubocop gem and GH actions
Passing:
https://github.com/ezr-ondrej/smart_proxy_ansible/actions/runs/399558335
https://github.com/ezr-ondrej/smart_proxy_ansible/actions/runs/404608706
In this PR, thanks @ekohl :)

@ezr-ondrej ezr-ondrej force-pushed the gh_actions branch 11 times, most recently from a701108 to 5a2ae4f Compare December 3, 2020 23:59
@ekohl
Copy link
Member

ekohl commented Dec 4, 2020

[test smart_proxy_ansible]

1 similar comment
@ekohl
Copy link
Member

ekohl commented Dec 4, 2020

[test smart_proxy_ansible]

ekohl
ekohl requested changes Dec 4, 2020
View reviewed changes
lib/smart_proxy_ansible/roles_reader.rb Outdated
Comment on lines 40 to 42
roles_path, 'roles' do
Dir.glob("#{roles_path}/*").map do |path|
path.split('/').last
end
Copy link
Member

Choose a reason for hiding this comment

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

This indenting looks odd

Gemfile Show resolved Hide resolved
@ezr-ondrej ezr-ondrej force-pushed the gh_actions branch 3 times, most recently from 7e96ba1 to eb55e98 Compare December 6, 2020 22:40
ekohl
ekohl reviewed Dec 6, 2020
View reviewed changes
Gemfile Outdated
gem 'pry'

if ENV.fetch('BUNDLE_SMART_PROXY', '1') == '1'
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about ENV['SMART_PROXY_PATH'] which is the path to pass to gem 'smart_proxy'

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, RIGHT! 👍

@ezr-ondrej ezr-ondrej force-pushed the gh_actions branch 5 times, most recently from 05045d9 to 7f88fbb Compare December 6, 2020 23:01
@ezr-ondrej
Copy link
Member Author

Added new passing tests, it looks better now ;)

ekohl
ekohl reviewed Dec 7, 2020
View reviewed changes
.github/workflows/ci.yml Outdated
Comment on lines 21 to 52
test_ruby:
runs-on: ubuntu-latest
needs: rubocop
env:
SMART_PROXY_PATH: '../smart-proxy'
strategy:
fail-fast: true
matrix:
smart-proxy-branch: [develop]
ruby-version: [2.5, 2.7]
steps:
- uses: actions/checkout@v2
with:
repository: theforeman/smart-proxy
ref: ${{ matrix.smart-proxy-branch }}
path: smart-proxy
- uses: actions/checkout@v2
with:
path: smart_proxy_ansible
- name: Setup Ruby
uses: ruby/setup-ruby@v1
with:
ruby-version: ${{ matrix.ruby-version }}
bundler-cache: true
working-directory: smart_proxy_ansible
- name: Create logs dir
working-directory: smart_proxy_ansible
run: mkdir logs
- name: Run plugin tests
working-directory: smart_proxy_ansible
run: |
bundle exec rake test
Copy link
Member

Choose a reason for hiding this comment

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

FYI: I already enabled testing on Jenkins so this is redundant now.

Copy link
Member Author

Choose a reason for hiding this comment

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

nice work 👍

@ekohl
Copy link
Member

ekohl commented Dec 8, 2020

FYI: you can trigger the tests by pushing a branch to Github with these actions. You can then delete the branch and get back to your fork. Then they show up in this PR.

ekohl
ekohl reviewed Dec 8, 2020
View reviewed changes
Gemfile

if (smart_proxy_path = ENV.fetch('SMART_PROXY_PATH', nil))
gem 'smart_proxy', path: smart_proxy_path
elsif ENV.fetch('BUNDLE_SMART_PROXY', '1') != '0'
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well you might want to completely disable bundling it, eg for the rubocop.

ekohl
ekohl reviewed Dec 8, 2020
View reviewed changes
.github/workflows/ci.yml
rubocop:
runs-on: ubuntu-latest
env:
BUNDLE_SMART_PROXY: '0'
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't an empty string work just as well?

Also, you can set BUNDLE_WITHOUT: 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.

That would not work IMO, the smart-proxy is not in development and on contrary, we want to install some development dependencies.

ekohl
ekohl reviewed Dec 23, 2020
View reviewed changes
lib/smart_proxy_ansible/plugin.rb Outdated
Comment on lines 5 to 8
http_rackup_path File.expand_path('http_config.ru',
File.expand_path('../', __FILE__))
File.expand_path(__dir__))
https_rackup_path File.expand_path('http_config.ru',
File.expand_path('../', __FILE__))
File.expand_path(__dir__))
Copy link
Member

Choose a reason for hiding this comment

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

I also think the double call to File.expand_path is redundant.

If you rely on Smart Proxy 2.3 or newer, you can use rackup_path File.expand_path('http_config.ru', __dir__) instead of the two separate calls.

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 got no idea if it's safe to assume that and the only dependency is development dependency 🤔
Although it would be nice to fix it, I don't think it should be part of this 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, but just wanted you to know. You can at least simplify it further with https_rackup_path File.expand_path('http_config.ru', __dir__). The extra File.expand_path doesn't really add anything.

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 put it on one line, and I'd like to postpone it after the merge of this.

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 lets have a PR for it not to forget: #25 :)

@ezr-ondrej ezr-ondrej force-pushed the gh_actions branch 2 times, most recently from 6b19441 to 290ea44 Compare December 23, 2020 14:24
@ezr-ondrej ezr-ondrej mentioned this pull request Jun 12, 2021
Merged
@ezr-ondrej ezr-ondrej force-pushed the gh_actions branch 7 times, most recently from 4bd3463 to 75a8267 Compare June 14, 2021 10:20
@adamruzicka
Copy link
Contributor

What's the plan here?

@ezr-ondrej ezr-ondrej force-pushed the gh_actions branch 2 times, most recently from 0423359 to ad9689d Compare November 10, 2021 21:06
@ezr-ondrej
Copy link
Member Author

It does weird stuff. Tho the question is, do we want this? :)

@adamruzicka
Copy link
Contributor

That depends. What was the original reason for this?

@ezr-ondrej
Copy link
Member Author

That depends. What was the original reason for this?

I guess add rubocop in GH actions to have inline RuboCop... probably was supposed to be followed up by GH action tests

@ekohl ekohl mentioned this pull request Sep 9, 2022
Open
@ekohl
Copy link
Member

ekohl commented Sep 9, 2022

Taking a new stab at this: #68.

@ekohl ekohl closed this Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekohl ekohl ekohl requested changes

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

Successfully merging this pull request may close these issues.
3 participants
@ezr-ondrej @ekohl @adamruzicka