Tails

#1 Updated by kytv 2015-05-04 11:17:01

• related to Bug #9233: Seahorse's configured keyservers are not the same as those in gpg.conf added

#2 Updated by kytv 2015-05-04 11:31:05

• Assignee changed from kytv to anonym
• QA Check set to Ready for QA
• Feature Branch set to kytv:test/9339-seahorse-keyserver

#3 Updated by intrigeri 2015-05-04 13:19:33

• blocks #8538 added

#4 Updated by anonym 2015-05-27 09:39:12

• Assignee changed from anonym to kytv
• QA Check changed from Ready for QA to Dev Needed

IMHO we should refrain from adding stuff into checks.feature (see Feature #5707, and it will truly become simple to split it as much as we want when Feature #6094 is finished) when there are places with at least some relation to what’s being tested. Isn’t torified_gnupg.feature more suitable for Scenario: Seahorse is configured to use the correct keyserver? If so, please move the step definition to torified_gnupg.rb as well.

> + assert_equal(1, gnome_keyserver.count, ‘Seahorse only have one keyserver configured.’)

The message is confusing. If I got that error I would think that it’s a problem that only one key server is configured. I think there’s a missing “must” or “should” in front of “only”.

+  gnome_keyserver = @vm.execute_successfully('gsettings get org.gnome.crypto.pgp keyservers',
+                                             LIVE_USER).stdout.chomp.gsub(/[\s'"\[\]]/,'').split(',')

The .stdout.chomp.gsub(...).split can be simplified since the output we get is yaml (or at least is compatible with it), so we can get the array simply with

@gnome_keyservers = YAML.load(@vm.execute_successfully('gsettings get org.gnome.crypto.pgp keyservers', LIVE_USER).stdout)

instead. Let’s avoid doing ad-hoc parsing when we can! Also note that I changed the variable name to the plural version since we could get more than one (in the failure case).

Other than this nitpicking, this scenario does the job.

#6 Updated by kytv 2015-05-27 22:32:54

• Assignee changed from kytv to anonym
• QA Check changed from Dev Needed to Ready for QA

anonym wrote:

>
> > + assert_equal(1, gnome_keyserver.count, ‘Seahorse only have one keyserver configured.’)
>
> The message is confusing. If I got that error I would think that it’s a problem that only one key server is configured. I think there’s a missing “must” or “should” in front of “only”.

That’s what was intended. How I missed that I dunno. :|

>
> […]
>
> The .stdout.chomp.gsub(...).split can be simplified since the output we get is yaml (or at least is compatible with it), so we can get the array simply with

Thanks for that pointer, I didn’t realize YAML.load would work.

I force pushed a fixed version, mostly force-pushing because I thought moving everything to torified_gnupg.* might be messier than if I just redid it. I can restore the original branch and make the modifications there if it’d be preferred.

#8 Updated by anonym 2015-05-28 15:44:48

• Status changed from In Progress to Fix committed
• % Done changed from 30 to 100

Applied in changeset commit:75a4e7b8f1c5d9f923e668fe12757074c1e8f141.

#9 Updated by anonym 2015-05-28 15:45:54

• Assignee deleted (anonym)
• QA Check changed from Ready for QA to Pass

It looks great now. Merged!

However, I have a suggestion:

+  assert_equal(CONFIGURED_KEYSERVER_HOSTNAME.gsub('hkps.', 'hkp://'), @gnome_keyservers[0])

Just some general notes about gsub() and sub(), which you are fond of using (which is completely fine, btw!):

1. We don’t need gsub() here and should use sub() since we only expect one replacement. More than one would cause issues.
2. Both sub() and gsub() can take a regex as the replacement pattern, and I think it’s good practice to anchor the patterns to where we expect them when possible, e.g. @.sub(/^hkps\./, ‘hkp://’) in this case.

I won’t block on this here since I cannot come up with a situation where this would go unnoticed and we would get a false positive, but please take it into consideration in the future.

#11 Updated by intrigeri 2015-07-03 03:40:24

• Status changed from Fix committed to Resolved