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.

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

Add rspec-github integration #353

Merged
merged 1 commit into from
Oct 24, 2022

Conversation

ekohl
Copy link
Contributor

@ekohl ekohl commented Aug 5, 2022

This adds rspec-github as a dependency and at runtime it is optionally configured. The detection is the same as for Rubocop.

In voxpupuli/puppet-example#22 I've showcased that this works:
Screen Shot 2022-08-05 at 15 44 53

@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2022

Codecov Report

Merging #353 (af049ed) into main (cbad6f4) will decrease coverage by 0.79%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main     #353      +/-   ##
==========================================
- Coverage   43.28%   42.49%   -0.80%     
==========================================
  Files          11       10       -1     
  Lines         841      779      -62     
==========================================
- Hits          364      331      -33     
+ Misses        477      448      -29     
Impacted Files Coverage Δ
lib/puppetlabs_spec_helper/module_spec_helper.rb 0.00% <0.00%> (ø)
lib/puppetlabs_spec_helper/puppet_spec_helper.rb 65.51% <ø> (ø)
lib/puppetlabs_spec_helper/tasks/fixtures.rb 38.36% <ø> (ø)
lib/puppetlabs_spec_helper/rake_tasks.rb 42.04% <54.16%> (+0.57%) ⬆️
...bs_spec_helper/puppetlabs_spec/puppet_internals.rb 92.50% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -26,6 +26,7 @@ Gem::Specification.new do |spec|
spec.add_runtime_dependency 'pathspec', '>= 0.2.1', '< 1.1.0'
spec.add_runtime_dependency 'puppet-lint', '~> 2.0'
spec.add_runtime_dependency 'puppet-syntax', ['>= 2.0', '< 4']
spec.add_runtime_dependency 'rspec-github', '~> 2.0'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As an alternative to making it a hard dependency, it can also be made a soft dependency:

RSpec.configure do |c|
  if ENV['GITHUB_ACTIONS'] == 'true'
    begin
      c.formatter = 'RSpec::Github::Formatter'
    rescue LoadError
      # rspec-github not present
    end
  end

Copy link

Choose a reason for hiding this comment

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

Does it really matter if it is a hard dep? As long as there is a flag to turn it off for other CI systems, I don't see the gem being pulled as much of a problem.

Copy link

Choose a reason for hiding this comment

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

If it isn't a hard dep, that would mean that rspec-github would have to be manually added to (literally) 100s of repos.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it really matter if it is a hard dep? As long as there is a flag to turn it off for other CI systems, I don't see the gem being pulled as much of a problem.

Every additional gem is a potential problem. Just wanted to highlight it.

If it isn't a hard dep, that would mean that rspec-github would have to be manually added to (literally) 100s of repos.

That was also my consideration. This looked easier for the average case.

Copy link
Collaborator

@bastelfreak bastelfreak left a comment

Choose a reason for hiding this comment

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

I had a discussion with @binford2k yesterday. I think this is a sane and useful patch. I also think rspec-github should be added as hard dependency, not a soft one. The patch was tested at Vox pupuli and it's working fine.

@chelnak
Copy link
Contributor

chelnak commented Oct 24, 2022

@ekohl I love this.

There are going to be a few more updates to the repo so this PR will need rebasing soon.. (unless you want to keep rebasing as we merge changes).

@chelnak
Copy link
Contributor

chelnak commented Oct 24, 2022

@ekohl @bastelfreak The bulk of work has been merged to main now. Would it be possible for one of you to rebase this and resolve the conflict? Once that is sorted i'll get it merged & it will be in the next release.

This adds rspec-github as a dependency and at runtime it is optionally
configured. The detection is the same as for Rubocop.
@ekohl ekohl force-pushed the add-rspec-github-integration branch from af049ed to 8eaa209 Compare October 24, 2022 16:13
@chelnak chelnak self-assigned this Oct 24, 2022
Copy link
Contributor

@chelnak chelnak left a comment

Choose a reason for hiding this comment

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

Exciting, thank you for this.

@chelnak chelnak merged commit c142acf into puppetlabs:main Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants