PR: Save last accepted kernel settings in config

[AfzalivE]

Pull Request Checklist

• Read and followed this repo's Contributing Guidelines
• Based this PR on the latest version of the correct branch (master or 3.x)
• Followed PEP 8 code style
• Ensured this pull request hasn't eliminated unrelated blank lines/spaces,
  modified the spyder/defaults directory, or added new icons/assets
• Wrote at least one-line docstrings following PEP 257for any new functions
• Added at least one unit test covering the changes, if at all possible
• Described the changes and the motivation for them below
• Noted what issue(s) this pull request resolves, creating one if needed
• Included a screenshot, if this PR makes any visible changes to the UI

Developer Certificate of Origin Affirmation

By submitting this Pull Request or typing my name below, I affirm the
Developer Certificate of Origin
with respect to both the content of the contribution itself and this post,
and understand I am releasing it under Spyder's MIT (Expat) license.

I certify the above statement is true and correct:
afzalive

Description of Changes

I use remote kernels quite often and every time I start a new kernel on a remote machine, I have to fill in all the fields. So finally, this change allows saving these fields so all you have to do is press enter.

In a new section called "existing-kernel" in the CONF, I'm saving the following fields when the user has checked "Save connection settings" checkbox and clicks "Ok" in the existing kernel dialog:

• json_file_path (default: blank)
• is_remote (default: unchecked)
• username (default: blank)
• hostname (default: blank)
• is_ssh_keyfile (default: password radio selected, so false)
• port (default: 22)
• ssh_key_file_path (default: blank)

And then populating these fields when the dialog is created (init). If these fields don't already exist in the CONF, default values (as mentioned above are used).

The following fields are saved in the keyring, under "spyder_remote_kernel":

• ssh password
• ssh key passphrase

There are 4 test cases to test this feature:

• Save user input and ssh key passphrase when save checkbox is checked
• Do not save user input and ssh key passphrase when save checkbox is not checked
• Save user input and ssh key password when save checkbox is checked
• Do not Save user input and ssh key password when save checkbox is not checked

Screenshot

Issue(s) Resolved

Fixes #3689

Thank you

[pep8speaks]

Hello @AfzalivE! Thanks for updating the PR.

• There are no PEP8 issues in the file spyder/plugins/ipythonconsole/widgets/kernelconnect.py !

• There are no PEP8 issues in the file spyder/plugins/ipythonconsole/widgets/tests/test_kernelconnect.py !

Comment last updated on November 28, 2018 at 07:48 Hours UTC

[AfzalivE]

@ccordoba12 Hope you can look into it

[bcolsen]

@AfzalivE Thanks for looking into this. Connecting to external kernel still has some rough corners in Spyder currently so your help is appreciated.

I've look over your changes and in general it looks like you're on the right track.

A few suggestions:

1. If you save the connection parameters as a dictionary in the config file so it will be easier to extend this code to multiple saved connections in the future.
2. If you use a dictionary you can clean up the code a bit by putting the setText calls for the fields in the same place towards the end under a single if statement checking for the dictionary
3. Saving should be optional with a check box option to save the connection in the dialog.

[AfzalivE]

Thanks @bcolsen.

Switched to dictionary. Great idea for future extensiblilty. Separated loading the values from CONF into a separate function that's executed at the end.

For (2), I don't think an if statement is needed to check the dictionary if defaults are used. So if the values don't exist, we get a blank dictionary, and for every get call, a default value is specified for that field. Is that okay?

Screenshot below for the "Save kernel settings" checkbox. So should they be saved when that checkbox is checks AND the user presses "OK"? Seems like no point in saving if the user presses "Cancel", right?

[CAM-Gerlach]

A few minor comments, but looks good so far. I haven't done functional testing yet; let me know when its ready for that.

Have you considered saving the password and/or passphrase using keyring, since we already use it for our Github dialog? The latter should provide a good template to follow on how to implement it, so it shouldn't be too tough.

Also, you'll want to write some unit tests for this; specifically, that the dialog remembers the various credentials when re-opened. Again, the tests for the Github dialog should be helpful.

Finally, as to the contents not being saved when the user clicks Cancel, I guess in general that's what would be intuitively expected by users (principle of least surprise), although there would be a at least a few potential use cases where saving on cancel would be useful.

[AfzalivE]

@CAM-Gerlach Addressed the comments and added a unit test.
@bcolsen Aligned the checkbox and the buttons.

[CAM-Gerlach]

Good job on the test! I only had some minor comments. Thanks!

Can you also test that the info is not being saved if the user unchecks the "remember" box?

[CAM-Gerlach]

If you're going to do a functional test, which is what the rest of this really is (which is great), its probably worth it to just close the dialog as the user would (by having Qtbot press the Ok button, and Mocking the call to open a new console so it doesn't actually open anything), rather doing all that setup in the GUI and then just explicitly calling save_connection_settings() Otherwise, either save_connection_settings() might not get called like it should, or something else could go wrong that's only triggered by the normal code path. Also, it makes sure the dialog is closed.

[AfzalivE]

So the problem here is that the normal code path calls the static method get_connection_parameters() which calls exec_() on the dialog and waits for user input. It seems like once I call get_connection_parameters() from the test code, the next line only executes after I manually dismiss the dialog.

Is there a way to run QtBot along with this static method that the normal code path calls?

[CAM-Gerlach]

If you just open the dialog with dialog.show() in your fixture, it shouldn't be a problem. Otherwise, there are a number of ways to handle modal dialogs as outlined in the Pytest documentation. The dialog probably shouldn't be modal to begin with, but that's a separate issue (that I've already opened).

[AfzalivE]

Hmm but get_connection_parameters() is the method that calls save_connection_settings() if it's accepted.

So, to be clear, it's okay if I don't call get_connection_parameters()? Maybe I'm misunderstanding what you actually asked to do in this comment (I must admit I'm not a full time python developer :p). Thanks

[CAM-Gerlach]

Hmm but get_connection_parameters() is the method that calls save_connection_settings() if it's accepted.

You could just call self.accept_btns.accepted.connect(self.save_connection_settings) here right before the connection to self.accept, instead of doing it afterward outside the dialog, so clicking OK saves the dialog contents first. You'd also need to include the check for self.save_layout.ischecked() as the first line of save_connection_settings(self) if you went that route, returning if not.

Saving the connection settings ideally should be presumably done as soon as the OK button is clicked, to prevent something else from going wrong first and the settings not being saved, as well as since (IMO) it is a property of the dialog itself, and should automatically be done whenever the dialog is accepted and the box is ticked, rather than being something external and specific to a way of opening the dialog—but I'm not a Qt or even really OOP expert, so perhaps @ccordoba12 or @jitseniesen know better than I do if this is really a good idea or not.

Alternatively (or in addition), you could just make the dialog non-modal (as it should be anyway) by calling .show() instead of .exec_() in get_connection_settings() but @ccordoba12 might consider that out of scope for this PR.

(I must admit I'm not a full time python developer :p)

Neither am I, if you couldn't tel already!

[CAM-Gerlach]

Evidently saving credentials is not working on Linux, so you could do a platform check with os.name or sys.platform, assuming its due to something with the OS's secure store rather than your code. @ccordoba12 which one are we using now, I thought we used to use sys.platform but I've seen os.name in newer tests? Also, insight into the specifics of why saving credentials is failing on Linux?

[bcolsen]

I tested the code and the test and both work well on Linux (Ubuntu 16.04).

I think the test are failing because the circleci environment don't have the correct setup for the desktop linux keyring tools like GNOME keyring or Kwallet. I'm not sure if there is anything we can do there. @ccordoba12 Any idea on this?

[CAM-Gerlach]

Thanks @bcolsen !

I think the test are failing because the circleci environment don't have the correct setup

To note, the Travis tests are failing also, presumably for the same reason.

[ccordoba12]

@ccordoba12 Any idea on this?

Yeah, please don't run keyring tests on Linux. They need packages not provided in Circle and Travis.

---

I must also say that I don't agree with the changes proposed in this PR (sorry @AfzalivE). What I want is a way to save any external kernel configuration settings, not just the last ones. That's because only saving the last settings is very limited and I'm sure users will ask us to expand this (if merged as it is) to include all kernels.

[AfzalivE]

@ccordoba12 Any idea on this?

Yeah, please don't run keyring tests on Linux. They need packages not provided in Circle and Travis.

I must also say that I don't agree with the changes proposed in this PR (sorry @AfzalivE). What I want is a way to save any external kernel configuration settings, not just the last ones. That's because only saving the last settings is very limited and I'm sure users will ask us to expand this (if merged as it is) to include all kernels.

I agree that this is very limited, but IMHO it's better than what we have right now, maybe there should be a Load Recent Setting button instead of automatically loading it. So this can be a starting point to iterate over and allow saving as many recent settings as people want. I'm perfectly fine with you not approving this PR if you don't like it :) (just means I'll to use my fork and keep it up-to-date :( )

The way I see it, after this, you could add a dropdown and show all saved kernel settings in there for a user to select from.

I'll try to fix the other concerns later this week.

[CAM-Gerlach]

BTW, I tested the changes in action on Windows, and they do work as advertised.

FYI, our convention for checking for if the tests are not running on a Linux CI is

(not sys.platform.startswith('linux')) or (not os.environ.get('CI') is not None))

so you can only do the password/passphrase asserts if that condition is true.

maybe there should be a Load Recent Setting

Alternatively, we could have a Clear button that resets the dialog to be blank (except with SSH port at 22 as default, and it wouldn't affect the saved settings), which would allow the user to more easily switch between a primary set of settings and multiple temporarily manually entered ones. For the use case of only one commonly used remote kernel, having an explicit "Load Last Saved Settings" button that needed to be pressed every single time would significantly harm usability since instead of just hitting "Enter" to connect to the kernel, the user would need to switch to the mouse, navigate to and click "Load.." then either navigate to and click "Connect" or switch back to the keyboard and hit Enter. Whereas, if they wanted to clear them then enter new ones, its only one more click to hit Clear. If the user wanted to recall their previous settings again for some reason after clearing the dialog, they could simply close and re-open it.

I must also say that I don't agree with the changes proposed in this PR (sorry @AfzalivE).

There's actually an open, milestoned issue #3689 already that requests the dialog remember just the last used settings, and that was also what the several users originally requesting this all asked for, a "remember" checkbox. This PR could be set to close that issue instead of #8052 , and then later either this contributor or someone else could come along and build upon this to add the requested multiple save/load UI and functionality to resolve the latter.

I agree it would be really nice to be able to save multiple sets of settings and recall them, but that is a non-trivial step in UI and UX complexity over the present form (not only requiring a dropdown to select one, but actually making that a combobox to allow the current one to be named/renamed, and as well as a button to trigger saving the current settings, and ideally a way to remove existing settings), whereas the current implementation already covers a large fraction, quite possibly even a majority of the intended use cases (the original requester, as well as those I've seen elsewhere outside of the core dev team, only asked to remember the last settings, and that was also what I was asking for, whereas even in scenarios where the user is switching between multiple external kernels it would still provide some utility).

Particularly since we've established a precedent of merging an initial, even incomplete implementation of a feature or enhancement that doesn't cover all the eventual intended use cases/functionality and then iterating on it (dark theme, autosave, docs theme, etc), I don't see how it would make any sense to reject this simply for not including a further extension to what has been actually asked for by the original requester when its already very useful on its own and a strict improvement on our current functionality.

[ccordoba12]

but IMHO it's better than what we have right now

Sure, it's better.

So this can be a starting point to iterate over and allow saving as many recent settings as people want

Ok, let's go with this solution first to see what users think about it.

@AfzalivE, please address the reviews in this PR. When I have time I'll give you my comments too.

[ccordoba12]

You also probably need to merge with master to fix the failing tests.

[CAM-Gerlach]

Good job with the changes I requested, just some minor feedback except for one major issue: You essentially copied and pasted a huge block of test code almost verbatim four times, which is a big no-no. Instead, you should incorporate all the setup code from cf_path = ... to at least right before # check save connection settings, if not including that block as well, into the fixture, and pass any variable settings to .get().

[AfzalivE]

@CAM-Gerlach Took the common code a bit further, let me know if it's too much or still ok.

[CAM-Gerlach]

A few substantive comments and a handful of small nitpicks. Thanks so much for your work thus far!

You could potentially put the asserts checking each field into a helper function, but not sure its worth it.

[AfzalivE]

Ok @CAM-Gerlach, I think I've spent a fair bit of time on this now and it seems alright. I'm not sure any minor typo fixes in unit tests are worth it, personally. Thanks for the comments, I've learned quite a bit about unit testing with Python!

[CAM-Gerlach]

I tested it again locally, and it seems to work and look great! Just one final tweak (x2 test functions) and it should be good to go from me, though @ccordoba12 will review it as well.

I didn't comment on the same unfixed grammar error (remember/remembers) in the docstrings, per your request; they should follow the test function names. Again, the English convention (weird, I know) is that you generally put an s after the verb in the third-person singular present tense "She runs, He bikes, The dialog remembers", but when you have "does/does not" in front, "does" is actually the verb so it get the s: "He does not run, She does bike, The dialog doesn't remember". So its "The dialog remembers " and "The dialog does/doesn't remember ".

[CAM-Gerlach]

Looks good from my end! Just awaiting @ccordoba12 's review. Thanks so much for your hard work and diligent effort @AfzalivE !

[ccordoba12]

@AfzalivE, could you modifiy the initial PR description and upload an image with the latest changes? Thanks!

[ccordoba12]

I left some minor comments, but otherwise it looks good. Thanks @AfzalivE!

[AfzalivE]

[AfzalivE]

@AfzalivE, could you modifiy the initial PR description and upload an image with the latest changes? Thanks!

Done. Updated the description according to the latest changes and uploaded a new image. Thanks!

[CAM-Gerlach]

@AfzalivE I added the screenshot to the main PR description, FYI. Your changes to address @ccordoba12 's concerns look good. Thanks!

[ccordoba12]

Thanks a lot @AfzalivE for your contribution!

[CAM-Gerlach]

100 comments later...thanks for sticking with it @AfzalivE !

[AfzalivE]

Worth it! Can't tell you how happy I was when I read that Spyder development had ramped up again!

[ccordoba12]

Actually, it never stopped ;-)

[CAM-Gerlach]

Mostly thanks to the single-handed efforts of @ccordoba12 !