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

Relax highline dependency to allow versions 1.6.X #19

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

Conversation

severin
Copy link

@severin severin commented Dec 5, 2024

Hey @ahoward 👋

In the process of upgrading a legacy code base to ruby 3.1 I noticed an issue with sekrets/openssl. The code base is running sekrets 1.13 and I noticed that sekrets 1.14 would fix the issue.

Unfortunately sekrets 1.14 also bumped the highline dependency from 1.6 to 1.7 and our code base has a hard dependency on highline 1.6, so we currently cannot use sekrets 1.14.

With this PR I propose to relax the highline dependency to allow for highline versions 1.6.X again. My reasons for doing so are (besides being more "inclusive" and allowing us to use 1.14 😉):

  • sekrets does not depend on any added functionality in highline 1.7
  • highline 1.7 did not fix any security issues present in 1.6 that would make an upgrade "mandatory"

In general I think gems should be as loose as possible with dependencies on other gems.

Let me know if this makes sense to you!

@UnderpantsGnome
Copy link
Contributor

@severin I might go one step further and remove highline I was going to submit a PR but it looks like Ara has been doing work for the next version on main

This should be enough to remove highline completely.

  def Sekrets.ask(question)
    print prompt_for(question)
    gets.strip
  end

@severin
Copy link
Author

severin commented Dec 12, 2024

@UnderpantsGnome that's nice to hear 👍

@severin
Copy link
Author

severin commented Dec 12, 2024

@UnderpantsGnome sorry, I just checked main and the code using highline is still there. Are you or @ahoward working on removing it or were you suggesting that I should open a PR to make that change?

@UnderpantsGnome
Copy link
Contributor

@severin sorry, I was pointing out that it can be easily removed. I went ahead and opened a PR for it here

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.

2 participants