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

Update release process after working with bastelfreak #275

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ghoneycutt
Copy link
Member

No description provided.

@@ -20,13 +32,15 @@ ruby 1.8 support has been dropped.)
If necessary, run `bundle install` before continuing. If you want you can also only install the needed gems:

```bash
bundle install --path .vendor/ --without system_tests development
# You can add --jobs with the number of cores you have to run this much faster in parallel.
bundle install --path .vendor --jobs 8
Copy link
Member

Choose a reason for hiding this comment

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

nproc helps with this on machines with coreutils (Linux):

Suggested change
bundle install --path .vendor --jobs 8
bundle install --path .vendor --jobs $(nproc)

Copy link
Member

Choose a reason for hiding this comment

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

The --path option in bundler is deprecated. Instead, it's recommend to use bundle config to persistently set it.

Copy link
Member

Choose a reason for hiding this comment

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

Also, setting the path to something like ~/.vendor avoid polluting the "repository" and also allows to only download once the dependencies when working with multiple modules.

Copy link
Member

Choose a reason for hiding this comment

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

So for me that's the default, so I never understood why people want to use .vendor. I have this snippet in my ~/.bashrc:

# https://github.com/rubygems/rubygems/issues/4031
# https://bugzilla.redhat.com/show_bug.cgi?id=1574594
if command -v ruby > /dev/null ; then
        GEM_HOME="$(ruby -e 'print Gem.user_dir')"
        export GEM_HOME
fi

Nowadays that resolves to ~/.gem/ruby. I will note that I have to unset it when I use rbenv because it messes up, but I never set the bundler path. IMHO the biggest benefit is that you do get the isolation from bundler, but can reuse gems between different modules.

Copy link
Member

Choose a reason for hiding this comment

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

That. Looks. Awesome 😍 ! That resolves in fact to ~/.gem/ruby/<RUBY VERSION> which means that when your version of Ruby change (on rolling releases OS), you don't have to choose between fighting with gems that needs to be rebuilt or removing the whole bundler directory, because. This add one more indirection level that might be confusing for beginners, but that looks so much better (I was not aware of this and tuned my config accordingly and I am very happy) 💯 !

Copy link
Member

Choose a reason for hiding this comment

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

Yes, those are great and in rubygems/rubygems#5878 there are many tips on how to make it default. And there's a general consensus that it should be default, but the concrete implementation has yet to surface (IIRC there is a PR in progress).

```

And in case you installed the gems before:

```bash
bundle install --path .vendor/ --without system_tests development; bundle update; bundle clean
rm -fr .vendor
Copy link
Member

Choose a reason for hiding this comment

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

If you're purging (and I really think you shouldn't need to), you should remove Gemfile.lock. In general I find removing Gemfile.lock is more useful than removing the installed gems.

I'd say we should recommend to only do this in case there are problems.

Copy link
Member

Choose a reason for hiding this comment

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

If bundling to a directory outside of the project, it should be enough to update the bundle:

Suggested change
rm -fr .vendor
bundle update

I had a few times to rm Gemfile.lock before installing again in the past, but I think it's much a bug of older versions of bundler rather than something we should recommend people to do? Maybe we can split this in 2:

  1. How to update;
  2. What to do if something goes wrong.

Copy link
Member

Choose a reason for hiding this comment

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

I still have it every now and then, but it's getting less frequent yet. 👍 for splitting it as you suggested.

@@ -20,13 +32,15 @@ ruby 1.8 support has been dropped.)
If necessary, run `bundle install` before continuing. If you want you can also only install the needed gems:

```bash
bundle install --path .vendor/ --without system_tests development
# You can add --jobs with the number of cores you have to run this much faster in parallel.
bundle install --path .vendor --jobs 8
Copy link
Member

Choose a reason for hiding this comment

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

The --path option in bundler is deprecated. Instead, it's recommend to use bundle config to persistently set it.

@@ -35,7 +49,7 @@ documentation is the existence of a REFERENCE.md file. You can automatically
generate the documentation by running the following rake task:

```bash
bundle exec rake strings:generate:reference
bundle exec rake reference
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is the way to go. Today we do have this task (https://github.com/voxpupuli/modulesync_config/blob/1e4e1e7d3a292f36c7043c481b868c56138b99e5/moduleroot/Rakefile.erb#L40-L44) but I think we only had one place where we used it (extlib).

I'm leaning to removing our custom rake task and only use the puppet-strings provided one. An additional reason is that the rake task in puppetlabs_spec_helper also calls the upstream provided one, meaning our custom one is likely broken in some situations. See https://github.com/puppetlabs/puppetlabs_spec_helper/blob/0bdac3fdafc456f25257ec7fa633c51df169a86a/lib/puppetlabs_spec_helper/rake_tasks.rb#L211-L217 for the validate task.

```bash
git checkout master; git fetch origin; git pull origin master
```
If you do the merge, you should do the following release rake task.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should recommend against merge commits for release PRs. My main reason is that you end up with tagging the merge commit this way, which I think is ugly. So either rebase (preferred) or squash merging.

Copy link
Member

Choose a reason for hiding this comment

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

On the other hand, when doing rebase/squash from the GitHub GUI, that means commits or rewritten and signed commits cannot be verified anymore. "Merging" the branch from git (git merge --ff-only release) and pushing should close the related PR I think, not as convenient as clicking "Merge" but might be a workaround?

Copy link
Member

Choose a reason for hiding this comment

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

Right, I didn't want to suggest we always merge via command line, but I use https://github.com/theforeman/foreman-installer-modulesync/blob/master/bin/release-merge. Note the actual releasing of the module is a bit different than what Vox Pupuli does, but https://github.com/theforeman/foreman-installer-modulesync/blob/master/bin/release-module is there for reference. And https://github.com/theforeman/foreman-installer-modulesync/blob/master/bin/release-pr is how I create the release PRs themselves. There are some git aliases in there that you may not have, but I think it should be mostly clear.

Note there are some things like passing in branches, which is there to maintain stable branches which Vox doesn't do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants