Skip to content

Improve dev env experience after Ruby version updates#11670

Closed
h-m-m wants to merge 1 commit intomainfrom
hmm/ruby-3.3.6/dev-config-nice-to-haves
Closed

Improve dev env experience after Ruby version updates#11670
h-m-m wants to merge 1 commit intomainfrom
hmm/ruby-3.3.6/dev-config-nice-to-haves

Conversation

@h-m-m
Copy link
Contributor

@h-m-m h-m-m commented Dec 18, 2024

🛠 Summary of changes

After #11605 was merged, @ajfarkas and I noticed that a couple small changes may help developers update their local dev environments.

  • Ruby projects usually have openssl@1.1 listed in their Brewfile to ensure older Ruby versions are able to build against it. I'm removing it assuming that's why it was added. Of note:
    • We're not on a Ruby version that requires it anymore
    • These days, tools like asdf and rbenv leverage the ruby-build Homebrew formula to manage Ruby-specific OpenSSL dependencies when installing older versions
  • Currently, local devs would only install the Foreman gem when running bin/setup, a script that also clobbers any local db entries. Adding the Foreman to the gemfile like this means that make update will also make sure an appropriate version of Foreman is always installed without dropping db tables

I was tempted to remove the Foreman-specific step from bin/setup entirely. I wasn't sure if it's called anywhere automatically, like referenced in an important Terraform script or the like, so I left it alone for now.

Gemfile Outdated
Copy link
Contributor

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

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

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.

changelog: Internal, Local Development, Improving development experience after recent Ruby version upgrade
@h-m-m h-m-m force-pushed the hmm/ruby-3.3.6/dev-config-nice-to-haves branch from 62df362 to 075bd96 Compare December 18, 2024 19:12
brew 'redis'
brew 'node@22'
brew 'yarn'
brew 'openssl@1.1'
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
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is impossible to do a new install of IDP locally because it is no longer available:

Error: openssl@1.1 has been disabled because it is not supported upstream! It was disabled on 2024-10-24.
Installing openssl@1.1 has failed!

Developers have been manually removing this line. It is not reasonable to keep this line, because it has not worked since October and will never work again.

OpenSSL 1 is no longer maintained except by paid support contract. We should probably stop using it in prod.

Copy link
Contributor

@mitchellhenke mitchellhenke left a comment

Choose a reason for hiding this comment

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

I should have done this is as a full review, but I added a couple comments that I consider blockers at the moment.

@ajfarkas
Copy link
Contributor

Additional context is that having openssl@1.1 in our Brewfile prevents successful install locally, with the following error:
Error: openssl@1.1 has been disabled because it is not supported upstream! It was disabled on 2024-10-24.

Copy link
Contributor

@n1zyy n1zyy left a comment

Choose a reason for hiding this comment

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

@mitchellhenke Would you consider lifting your "Changes requested" review? OpenSSL 1.1 is EOL and not available through homebrew.

@mitchellhenke
Copy link
Contributor

@mitchellhenke Would you consider lifting your "Changes requested" review? OpenSSL 1.1 is EOL and not available through homebrew.

The comments around Foreman are still I think unresolved

@h-m-m
Copy link
Contributor Author

h-m-m commented Jan 10, 2025

Closing in favor of #11731

@h-m-m h-m-m closed this Jan 10, 2025
@h-m-m h-m-m deleted the hmm/ruby-3.3.6/dev-config-nice-to-haves branch January 10, 2025 17:54
@n1zyy
Copy link
Contributor

n1zyy commented Jan 10, 2025

@mitchellhenke My bad, I missed that part. 👍

I have merged the fix removing openssl@1.1 in #11731 -- apologies for the small merge conflict that will cause.

@h-m-m
Copy link
Contributor Author

h-m-m commented Jan 10, 2025

The resolution of what to do about Foreman after Ruby version upgrades is not clear. I'm fine with opening a new PR when we have a good answer for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants