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

Improve dev env experience after Ruby version updates #11670

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

Improve dev env experience after Ruby version updates #11670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Brewfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,5 @@ brew 'postgresql@14'
brew 'redis'
brew 'node@22'
brew 'yarn'
brew '[email protected]'
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of our deployed environments still use openssl 1.1, so I think it'd be preferable to keep it as-is for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to resolve the fact that this prevents engs from deploying idp on their local environments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the dependency for the environments that still use openssl 1.1? Login.gov is very good about keeping important things like Ruby versions up to date, which is why this change felt safe to me.

brew 'jq'
cask 'chromedriver'
2 changes: 2 additions & 0 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ gem 'zxcvbn', '0.1.12'
group :development do
gem 'better_errors', '>= 2.5.1'
gem 'derailed_benchmarks'
# Putting foreman here saves developers from having bin/setup when the Ruby version changes
gem 'foreman', require: false
Copy link
Member

Choose a reason for hiding this comment

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

We might want to change the foreman invocation in the Makefile to bundle exec foreman in the case the gem is installed locally. However I think that will error out if the gem is installed system-wide.

Copy link
Contributor Author

@h-m-m h-m-m Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good point. In my environment, I tried adding this line to the Gemfile to see if using bundle exec foreman was required, and I didn't need it. I am using rbenv, and I'd like to see if anyone not using rbenv also finds this okay.

Here's what happened when I added it to the Gemfile before and after running `bundle install` 
➜  identity-idp git:(main) ✗ make run
yarn generate-browsers-json
yarn run v1.22.22
$ ./scripts/generate-browsers-json.js
✨  Done in 0.19s.
foreman start -p 3000
rbenv: foreman: command not found

The `foreman' command exists in these Ruby versions:
  3.2.0
  3.2.2
  3.3.0
  3.3.1
  3.3.4

make: *** [run] Error 127
➜  identity-idp git:(main) ✗ bundle
Fetching gem metadata from https://rubygems.org/........
Resolving dependencies...
Fetching foreman 0.88.1
Installing foreman 0.88.1
WARN: Unresolved or ambiguous specs during Gem::Specification.reset:
      stringio (>= 0)
      Available/installed versions of this gem:
      - 3.1.2
      - 3.1.1
WARN: Clearing out unresolved specs. Try 'gem cleanup <gem>'
Please report a bug if this causes problems.
Bundle complete! 127 Gemfile dependencies, 279 gems now installed.
Gems in the groups 'deploy' and 'production' were not installed.
Use `bundle info [gemname]` to see where a bundled gem is installed.
➜  identity-idp git:(main) ✗ make run
foreman start -p 3000
13:22:49 web.1    | started with pid 5871
13:22:49 worker.1 | started with pid 5872
13:22:49 js.1     | started with pid 5873
13:22:49 css.1    | started with pid 5874
13:22:49 js.1     | yarn run v1.22.22
13:22:49 css.1    | yarn run v1.22.22

[etc]

Copy link
Contributor

@mitchellhenke mitchellhenke Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Foreman recommends against putting it in the Gemfile:

Ruby users should take care not to install foreman in their project's Gemfile. See this wiki article for more details.

The reasoning described in the wiki article seems sensible enough to me to not include it.

gem 'irb'
gem 'letter_opener', '~> 1.8'
gem 'rack-mini-profiler', '>= 1.1.3', require: false
Expand Down
2 changes: 2 additions & 0 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,7 @@ GEM
ffi-compiler (1.3.2)
ffi (>= 1.15.5)
rake
foreman (0.88.1)
foundation_emails (2.2.1.0)
fugit (1.11.1)
et-orbi (~> 1, >= 1.2.11)
Expand Down Expand Up @@ -794,6 +795,7 @@ DEPENDENCIES
faker
faraday (~> 2)
faraday-retry
foreman
foundation_emails
fugit
good_job (~> 4.0)
Expand Down