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

(maint) - Release prep v4.2.2 #178

Merged
merged 1 commit into from
Nov 21, 2023
Merged

(maint) - Release prep v4.2.2 #178

merged 1 commit into from
Nov 21, 2023

Conversation

jordanbreen28
Copy link

@jordanbreen28 jordanbreen28 commented Nov 21, 2023

Summary

Rolling back release v5.0.0.
Releasing puppet-lint gem with reference to newly published rubygem on GitHub registry.
Have added original owner back as this will be the final release under https://rubygems.org/gems/puppet-lint.

Additional Context

Add any additional context about the problem here.

  • Root cause and the steps to reproduce. (If applicable)
  • Thought process behind the implementation.

Related Issues (if any)

Mention any related issues or pull requests.

Checklist

  • 🟢 Spec tests.
  • 🟢 Acceptance tests.
  • Manually verified.

@david22swan david22swan marked this pull request as ready for review November 21, 2023 11:55
@david22swan david22swan requested review from bastelfreak and a team as code owners November 21, 2023 11:55
@david22swan david22swan merged commit e981b19 into main Nov 21, 2023
8 checks passed
@david22swan david22swan deleted the maint-release_prep branch November 21, 2023 11:55
@ekohl
Copy link

ekohl commented Nov 21, 2023

Have added original owner back as this will be the final release under https://rubygems.org/gems/puppet-lint.

Would it make sense to publish to both?

Facter gets released as GPG signed to https://downloads.puppet.com/ which feels like the right thing to do here. In Fedora's Facter packaging I use that as the official source (and verify the GPG signature). IMHO that gives way more supply chain safety than publishing to GitHub and also consistency to the community.

@@ -2,12 +2,14 @@ $LOAD_PATH.push(File.expand_path('lib', __dir__))
require 'puppet-lint/version'

Gem::Specification.new do |spec|
spec.name = 'puppetlabs-puppet-lint'
spec.name = 'puppet-lint'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was merged quite fast. I would have suggested to revert the original commit instead. Or at least do the change in a dedicated PR and not the release PR. I think the renaming is a quite significant change and now it won't appear in the changelog?

@bastelfreak
Copy link
Collaborator

Have added original owner back as this will be the final release under https://rubygems.org/gems/puppet-lint.

Would it make sense to publish to both?

I assumed and I'm still hoping that the final gems will be released to github and rubygems. Vox Pupuli follows the same pattern and that was also our recommendation in the last Vox Pupuli monthly meeting.

@jordanbreen28
Copy link
Author

@bastelfreak @ekohl - Lets discuss. We're going to re-open the discussion

@ekohl
Copy link

ekohl commented Nov 21, 2023

A gemspec can only name a dependency, it can't indicate the source. If a user doesn't specify the source of puppet-lint in a Gemfile, they will get an older version. It also means a lot more work for everyone consuming puppet-lint to set up. If you trust Rubygems, it's just so much easier if you publish to both. Not having to think about transitive dependencies, all of that.

In the case of Vox Pupuli, we don't depend on puppet-lint directly. Instead, we pull it in via voxpupuli-puppet-lint-plugins which in turn pulls it in via voxpupuli-test. Our modules have something that comes down to:

group :test do
  gem 'voxpupuli-test', '~> 7.0', require: false
end

If you don't publish to Rubygems, we need to change this to:

group :test do
  gem 'voxpupuli-test', '~> 7.0', require: false
  source 'https://rubygems.pkg.github.com/puppetlabs' do
    gem 'puppet-lint'
  end 
end

Out of interest, I just tried to at least consume voxpupuli-test but then I see:

$ bundle update
Authentication is required for rubygems.pkg.github.com.
Please supply credentials for this source. You can do this by running:
`bundle config set --global rubygems.pkg.github.com username:password`
or by storing the credentials in the `BUNDLE_RUBYGEMS__PKG__GITHUB__COM` environment variable

So that will make consuming puppet-lint anywhere via Bundler a massive pain, especially in CI.

My recommendation:

  • Publish to Rubygems as has always been done
  • Publish to GitHub's registry
  • Publish GPG signed releases to downloads.puppet.com

For Fedora RPM packaging of Facter & Puppet I verify the GPG key (https://src.fedoraproject.org/rpms/facter/blob/rawhide/f/facter.spec#_64 & https://src.fedoraproject.org/rpms/puppet/blob/rawhide/f/puppet.spec#_64) and I'll do the same for puppet-lint if possible.

The PDK can then consume gems from a verified source, either via GH or downloads.puppet.com. I don't know the PDK build process, but perhaps just being able to verify a gem is signed and is identical to what's on Rubygems is sufficient assurance for customers.

Copy link

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Another note: the changelog does indicate 5.0.0:

## [v5.0.0](https://github.com/puppetlabs/puppet-lint/tree/v5.0.0) - 2023-10-18
[Full Changelog](https://github.com/puppetlabs/puppet-lint/compare/v4.2.1...v5.0.0)
### Changed
- (CAT-1248) Update Published Name and Owner [#158](https://github.com/puppetlabs/puppet-lint/pull/158) ([david22swan](https://github.com/david22swan))

No 4.2.2 entry exists.

As @bastelfreak said, a straight forward git revert would have been easier to follow.

@jordanbreen28
Copy link
Author

A gemspec can only name a dependency, it can't indicate the source. If a user doesn't specify the source of puppet-lint in a Gemfile, they will get an older version. It also means a lot more work for everyone consuming puppet-lint to set up. If you trust Rubygems, it's just so much easier if you publish to both. Not having to think about transitive dependencies, all of that.

In the case of Vox Pupuli, we don't depend on puppet-lint directly. Instead, we pull it in via voxpupuli-puppet-lint-plugins which in turn pulls it in via voxpupuli-test. Our modules have something that comes down to:

group :test do
  gem 'voxpupuli-test', '~> 7.0', require: false
end

If you don't publish to Rubygems, we need to change this to:

group :test do
  gem 'voxpupuli-test', '~> 7.0', require: false
  source 'https://rubygems.pkg.github.com/puppetlabs' do
    gem 'puppet-lint'
  end 
end

Out of interest, I just tried to at least consume voxpupuli-test but then I see:

$ bundle update
Authentication is required for rubygems.pkg.github.com.
Please supply credentials for this source. You can do this by running:
`bundle config set --global rubygems.pkg.github.com username:password`
or by storing the credentials in the `BUNDLE_RUBYGEMS__PKG__GITHUB__COM` environment variable

So that will make consuming puppet-lint anywhere via Bundler a massive pain, especially in CI.

My recommendation:

  • Publish to Rubygems as has always been done
  • Publish to GitHub's registry
  • Publish GPG signed releases to downloads.puppet.com

For Fedora RPM packaging of Facter & Puppet I verify the GPG key (https://src.fedoraproject.org/rpms/facter/blob/rawhide/f/facter.spec#_64 & https://src.fedoraproject.org/rpms/puppet/blob/rawhide/f/puppet.spec#_64) and I'll do the same for puppet-lint if possible.

The PDK can then consume gems from a verified source, either via GH or downloads.puppet.com. I don't know the PDK build process, but perhaps just being able to verify a gem is signed and is identical to what's on Rubygems is sufficient assurance for customers.

Thanks for the in-depth analysis @ekohl.
We have taken this all into account and will update the discussions thread by COB tomorrow with our final steps outlined.

RE The changelog, this was an oversight and will be corrected in the next release of puppet-lint.

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.

4 participants