Opened 7 years ago

Closed 7 years ago

#2449 closed enhancement (fixed)

Make needed things for bindctl readline support more clear

Reported by: shane Owned by: jinmei
Priority: medium Milestone: Sprint-20121120
Component: ~bind-ctl (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: High
Sub-Project: Core Feature Depending on Ticket: alpha2
Estimated Difficulty: 2 Add Hours to Ticket: 0
Total Hours: 0.75 Internal?: no

Description

We've had a number of users who installed and ran BIND 10 without readline support, because the needed Python module was missing. While we can run in this mode, we should try to let users know about readline support so it is available for them.

We can do two things:

  1. Add more detail to the documentation (one of the boxed "notes" perhaps)
  2. Output a message when bindctl starts saying something like "install readline module to use command history"

Subtickets

Change History (11)

comment:1 Changed 7 years ago by shane

  • Feature Depending on Ticket set to alpha2

comment:2 Changed 7 years ago by vorner

Hello

I think most people don't read documentation much, unless they have a problem.
And this would probably go into the installation documentation ‒ which most
people skip completely (at least I don't remember ever reading any installation
documentation).

So I'd vote for the second option.

comment:3 Changed 7 years ago by jelte

+1 on the warning/print

comment:4 Changed 7 years ago by shane

To be clear, I think we should do both. :)

comment:5 Changed 7 years ago by jelte

  • Defect Severity changed from N/A to High
  • Milestone changed from Next-Sprint-Proposed to Sprint-20121120

comment:6 Changed 7 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:7 follow-up: Changed 7 years ago by jinmei

I've committed my suggested solution to trac2449. Please review.

I couldn't come up with an effective way of testing this
automatically, so I didn't add an explicit unit test. (Although
indirectly, it would cause regression in some lettuce tests if we
show the warning even if stdin isn't a terminal, in which sense we
can at least know the code was used in the test). If there's any
effective and meaningful way of testing it explicitly I'll add it.

I think the documentation update should go to #2305. I'll make a
comment about it in that ticket.

I don't think we need a changelog for this.

comment:8 Changed 7 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:9 in reply to: ↑ 7 ; follow-up: Changed 7 years ago by muks

  • Owner changed from UnAssigned to jinmei

Replying to jinmei:

I've committed my suggested solution to trac2449. Please review.

It looks fine to me. Please go ahead and merge.

I couldn't come up with an effective way of testing this
automatically, so I didn't add an explicit unit test. (Although
indirectly, it would cause regression in some lettuce tests if we
show the warning even if stdin isn't a terminal, in which sense we
can at least know the code was used in the test). If there's any
effective and meaningful way of testing it explicitly I'll add it.

In this case, we can probably skip it. It doesn't seem to something that a test can help. If you must test it, you can move the stuff from the top of run() into a different method, and make a mock class deriving from BindCmdInterpreter which just sets a flag if that method is called. Then, set bindcmd.my_readline both ways, call run() (making self.login_to_cmdctl() fail and hence returning early) and see whether that flag is set or not. But I really think this much of testing is overkill for just printing a message.

I think the documentation update should go to #2305. I'll make a
comment about it in that ticket.

Verified that this has been added in trac2305.

I don't think we need a changelog for this.

Nod.

comment:10 in reply to: ↑ 9 Changed 7 years ago by jinmei

Replying to muks:

Replying to jinmei:

I've committed my suggested solution to trac2449. Please review.

It looks fine to me. Please go ahead and merge.

Thanks for the review (and comments), merge done, closing.

comment:11 Changed 7 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 0.75
Note: See TracTickets for help on using tickets.