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 to Highline 2.1.0 #201

Merged
merged 7 commits into from
Mar 21, 2023
Merged

Update to Highline 2.1.0 #201

merged 7 commits into from
Mar 21, 2023

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Jan 4, 2023

steal anything you want out of here @bdunne

not thrilled with the changes to tests with the expect heard stuff.
wish the questions were not going through, but this is the least invasive way I could think to solve

overlaps with #200

Fixes #181

@kbrock kbrock changed the title Update Highline gem Update to Highline 2.1.0 Jan 6, 2023
@bdunne bdunne mentioned this pull request Jan 9, 2023
spec/database_admin_spec.rb Outdated Show resolved Hide resolved
@kbrock
Copy link
Member Author

kbrock commented Jan 10, 2023

update:

  • fixed highline gem value
  • removed string interpolation in test
  • moved dependency to spot where dependencies were defined (it was not finding the files for some reason, but it may have been another problem. seemed good to me anyway)

I tested on a nightly appliance. had to muck with the appliance console gemspec in 2 places and the vmdb/gemfile.lock, but I was able to setup dhcp, create a v2 key, set the region and configure an internal database.

@kbrock
Copy link
Member Author

kbrock commented Jan 11, 2023

@bdunne let me know if you are happy with these changes.
as I mentioned I was able to change dhcp, and setup a basic appliance. If there are other configuration options you want me to test, let me know.

@kbrock
Copy link
Member Author

kbrock commented Mar 10, 2023

@Fryguy or @bdunne I did run through those few tests. Anything else we want to do to be comfortable merging?

Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

LGTM - @bdunne Please also review.

@Fryguy
Copy link
Member

Fryguy commented Mar 20, 2023

@kbrock Can you run the full rspec suite locally? We still don't have github actions working in this repository yet.

@Fryguy Fryguy mentioned this pull request Mar 21, 2023
5 tasks
@Fryguy Fryguy closed this Mar 21, 2023
@Fryguy Fryguy reopened this Mar 21, 2023
@Fryguy
Copy link
Member

Fryguy commented Mar 21, 2023

Bouncing now that GitHub Actions is in place - 3.0 should still fail cause of kwargs at the moment

@Fryguy
Copy link
Member

Fryguy commented Mar 21, 2023

Oh nice! This seems to fix the pending I put in spec/database_admin_spec.rb. Can you remove that pending in this PR @kbrock ?

The questions asked and the output are merged together.
This method was only used in one spot so not much lost.
If we enter a value that is not a default value, but one is derived, then the actual value entered is output.

so if we type in text that infers option 1, then highline now outputs (1)

Not exactly sure why there is a change for a default value being presented,
I'm guessing the value was not checked by the previous test. So the change in expect_heard
shows this at this time
This is replaced by HighLine.default_instance
this was necessary to get older version of highline up and running in rails
see ManageIQ#178 df5cffb
@kbrock
Copy link
Member Author

kbrock commented Mar 21, 2023

update:

  • removed the pending (ci ok)
    update:
  • removed the CI tty hack (broke ci)
    update
  • testing out alternative no-echo input

@miq-bot
Copy link
Member

miq-bot commented Mar 21, 2023

Checked commits kbrock/manageiq-appliance_console@5db7c89~...ff01d19 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
6 files checked, 0 offenses detected
Everything looks fine. ⭐

Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

LGTM

@Fryguy
Copy link
Member

Fryguy commented Mar 21, 2023

Love that you could kill the tty hack too

@Fryguy Fryguy merged commit 8544066 into ManageIQ:master Mar 21, 2023
@Fryguy Fryguy assigned Fryguy and unassigned bdunne Mar 21, 2023
@kbrock kbrock deleted the highline branch March 21, 2023 17:45
Fryguy added a commit that referenced this pull request Feb 7, 2024
Fixed
- Fix sporadic test failure [#204]
- Remove MIQ specific gem source [#209]
- Double escape @ in realm to avoid shell interpretation [#211]
- Move gem name loader to proper namespaced location [#208]
- Separate kerberos from service principal name and use correctly [#215]
- Add manageiq user to allowed_uids for sssd [#220]
- Remove warning about using pg_dump [#221]
- Fix specs where AwesomeSpawn private interface changed [#224]
- Change the Name of the CA from something to ApplianceCA [#228]
- Fix YAML.load_file failing on aliases [#234]

Added
- Make backward compatible changes to work with repmgr13 - version 5.2.1 [#192]
- Support Ruby 3.0 [#206]
- Support Ruby 3.1 [#227]
- Allow rails 7 gems in gemspec [#226]

Changed
- Update to Highline 2.1.0 [#201]
- Clean up test output (highline and stdout messages) [#210]

Removed
- Drop Ruby 2.7 [#223]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade highline gem
5 participants