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

Follow-up #3403: Add new option to bundle gem for choosing a CI service #3667

Merged
merged 26 commits into from Jun 9, 2020
Merged

Follow-up #3403: Add new option to bundle gem for choosing a CI service #3667

merged 26 commits into from Jun 9, 2020

Conversation

FTLam11
Copy link
Contributor

@FTLam11 FTLam11 commented May 29, 2020 •
edited

Please refer to #3403 for full details. This PR is more of a follow-up and mainly incorporates naming/grammar suggestions while addressing some test suite failures.

One thing that may warrant discussion: Test framework selection/configuration does not currently affect the generation of CI files. So a user can choose to not generate test framework files, while still being able to generate CI files.

Tasks:

  • Describe the problem / feature
  • Write tests
  • Write code to solve the problem
  • Get code review from coworkers / friends

I will abide by the code of conduct.

@bundlerbot bundlerbot added Bundler CI CI related issues labels May 29, 2020
@hsbt hsbt mentioned this pull request May 29, 2020
Closed
4 tasks
olleolleolle
olleolleolle approved these changes May 29, 2020
View reviewed changes
Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

Looks good!

bundler/spec/commands/newgem_spec.rb Outdated Show resolved Hide resolved
deivid-rodriguez
deivid-rodriguez reviewed May 29, 2020
View reviewed changes
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Looks good, I added a few minor comments.

Regarding the potential discussion, we kind of already had it at #3584. But I feel it becomes a bit too tricky to make all this logic dependent on the --test logic... 🤔.

bundler/lib/bundler/templates/newgem/.github/workflows/main.yml.tt Outdated Show resolved Hide resolved
bundler/lib/bundler/templates/newgem/.github/workflows/main.yml.tt Outdated Show resolved Hide resolved
bundler/lib/bundler/templates/newgem/.gitlab-ci.yml.tt Outdated Show resolved Hide resolved
bundler/lib/bundler/cli/gem.rb Show resolved Hide resolved
bundler/lib/bundler/cli.rb Outdated Show resolved Hide resolved
@FTLam11
Copy link
Contributor Author

FTLam11 commented May 30, 2020

I just remembered to also update bundle gem man pages for the new --ci option.

bronzdoc
bronzdoc reviewed May 31, 2020
View reviewed changes
bundler/lib/bundler/cli/gem.rb Outdated Show resolved Hide resolved
bundler/lib/bundler/cli/gem.rb Outdated Show resolved Hide resolved
bundler/lib/bundler/cli/gem.rb Outdated Show resolved Hide resolved
bundler/man/bundle-gem.1.txt Outdated Show resolved Hide resolved
deivid-rodriguez
deivid-rodriguez reviewed May 31, 2020
View reviewed changes
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Just some little things I noticed on the Github Actions configuration.

bundler/lib/bundler/templates/newgem/.github/workflows/main.yml.tt Outdated Show resolved Hide resolved
bundler/lib/bundler/templates/newgem/.github/workflows/main.yml.tt Outdated Show resolved Hide resolved
bundler/lib/bundler/templates/newgem/.github/workflows/main.yml.tt Outdated Show resolved Hide resolved
@deivid-rodriguez deivid-rodriguez mentioned this pull request Jun 2, 2020
Merged
4 tasks
deivid-rodriguez
deivid-rodriguez reviewed Jun 2, 2020
View reviewed changes
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment •
edited

Choose a reason for hiding this comment

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

Just one more round of comments. I believe it's the last one from me. Thanks for your patience, @FTLam11!

By the way, this PR uncovered an issue in rubygems' default bundler installer: #3674.

bundler/lib/bundler/cli/gem.rb Outdated Show resolved Hide resolved
bundler/lib/bundler/cli/gem.rb Show resolved Hide resolved
bundler/lib/bundler/templates/newgem/.circleci/config.yml.tt Outdated Show resolved Hide resolved
deivid-rodriguez
deivid-rodriguez requested changes Jun 3, 2020
View reviewed changes
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment •
edited

Choose a reason for hiding this comment

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

Found a little issue in the CircleCI template, and another little issue in the specs that made them not catch the bug in the template.

bundler/lib/bundler/templates/newgem/.circleci/config.yml.tt Outdated Show resolved Hide resolved
colby-swandale and others added 17 commits June 3, 2020 22:06
@colby-swandale @FTLam11
add new option for choosing a CI sevice in bundle gem command
80260b3
@colby-swandale @FTLam11
update question text
f14c8a5
@indirect @FTLam11
minor language tweaks, small code refactor
84e9f37
@FTLam11
Extract shared example for absence of CI files
22cb599
@FTLam11
Separate CI and test framework shared examples
afaecf1 
* `bundle gem` has new option to choose CI provider other than Travis CI
@FTLam11
Remove shared examples for present test framework
320f354 
* `bundle gem` has new option to select CI provider
@FTLam11
Move CI specs below test framework specs
d8e416d
@FTLam11
Move CI templates under bundler directory
e4fd4cc
@FTLam11
Incorporate naming/grammar suggestions from olleolleolle
8057145
@FTLam11
Update manifest with new CI templates
5db2419
@FTLam11 @olleolleolle
Fix CI spec typo
24f3739 
Co-authored-by: Olle Jonsson <olle.jonsson@gmail.com>
@FTLam11
Set CI global config to false in gem_helper_spec
39b18fe
@FTLam11
Update CI templates
229ad71 
* Next version of Bundler will auto choose jobs by default
* --retry 3 is already default
@FTLam11
Remove extra whitespace
9986cd0
@FTLam11
Make test framework/CI configuration for bundle gem consistent
5f779f4 
* Add hints for --ci option
@FTLam11
Rebuild man pages for bundle gem
b5a9cec
@FTLam11
Fix --ci flag manpage documentation
515effe
deivid-rodriguez
deivid-rodriguez approved these changes Jun 4, 2020
View reviewed changes
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

This looks good to me, @FTLam11! Outstanding work again 😍

Only thing I wonder is whether it'd be better to introduce #3685 first, and then you rebase and remove the last commit from this PR, so that we don't introduce changes that are about to be reverted.

@FTLam11
Copy link
Contributor Author

FTLam11 commented Jun 4, 2020

Only thing I wonder is whether it'd be better to introduce #3685 first, and then you rebase and remove the last commit from this PR, so that we don't introduce changes that are about to be reverted.

Sounds good, I'll be on standby. Thanks for giving really thorough reviews through this entire PR, it has been very helpful.

@hsbt hsbt merged commit 8949097 into rubygems:master Jun 9, 2020
@deivid-rodriguez
Copy link
Member

Hi @FTLam11, as you can see from the merge, we ended up doing this in a different order than I had planned, so you no longer need to mess with rebasing this PR :)

Just wanted to thank you for your hard work on improving our gem generator 💪.

@FTLam11 FTLam11 deleted the 3404-bundle-gem-ci branch June 9, 2020 22:43
hsbt added a commit to ruby/ruby that referenced this pull request Jun 18, 2020
@hsbt
Removed the old file with rubygems/rubygems#3667
be2dbfc
@marcandre marcandre mentioned this pull request Nov 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olleolleolle olleolleolle olleolleolle approved these changes

@hsbt hsbt hsbt approved these changes

@deivid-rodriguez deivid-rodriguez deivid-rodriguez approved these changes

@bronzdoc bronzdoc bronzdoc approved these changes

Assignees
No one assigned
Labels
Bundler CI CI related issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@FTLam11 @deivid-rodriguez @olleolleolle @hsbt @bronzdoc @bundlerbot @colby-swandale @indirect