Add acl comment and some build chagnes by macrotex · Pull Request #79 · rra/wallet

macrotex · 2020-02-18T05:41:44Z

These are the changes in this pull request:

Add the "comment" field to ACLs. Interface is the same as for objects:

  wallet acl comment group/access "This is a comment"

Output looks like this:

  Members of ACL group/spdb (id: 7338) are:
    krb5 host/lemon-uat1.example.com@EXAMPLE.COM
    krb5 host/lemon-uat2.example.com@EXAMPLE.COM
    krb5 host/lemon1.example.com@EXAMPLE.COM
    krb5 host/lemon2.example.com@EXAMPLE.COM
    netdb-root lemon-uat1.example.com
    netdb-root lemon-uat2.example.com
    netdb-root lemon1.example.com
    netdb-root lemon2.example.com
  comment: This is a comment

In the output of an ACLs history add secondary sort on the history entries' primary key in the history table. This is to handle cases where several entries have the identical create time.
Remove trailing white-space at the end of some of the lines of output of the "show" commands.
Force the timezone to be 'local' in DateTime->from_epoch function calls.
Re-order table drops in Wallet::Admin::destroy to take into account table dependencies.
In Wallet::Report make sure that acl memberships are sorted to fix a bug in the acl_duplicates method.
Add a 1-byte character set for the MySQL .sql file. Newer MySQL server releases default to a 4-byte character set when creating tables. Wallet sets indices on some 255-byte fields which, with a 4-byte character set, exceeds the maximum index length that MySQL supports.
Fix some tests to be more flexible when running "make check" against MySQL or PostgreSQL. MySQL and PostgreSQL can leave "holes" in the primary keys used if a record insert fails due to a constraint violation. Thus, the primary key for some ACLs in the test suite will differ when running "make check" against SQLite versus running "make check" against MySQL.
Turn off version checking when doing initial set up of a database. Otherwise DBIx::Class::Schema complains about an unversioned database.
Fix bug in t/general/admin.t test suite where some tests would fail if run with a backend other than SQLite.

revert version number

rra

I've cherry-picked some of the smaller changes (noted here) to reduce the size of the diff. The rest looks okay except for the time zone change, which I think is incorrect. Thank you for cleaning up all of those testing issues!

Could you please break the rest of this up into one commit per logical change (ideally one PR per logical change so that I can merge them independently, but I can handle them all in one PR if you want)? If each of your bullet points above should be a separate commit/PR, it should be easy to get the rest of this merged.

BTW, git commit --author doesn't override the committer and thus doesn't change one's identity. To change the committer identity, you need to use git config --local user.name and git config --local user.email.

rra · 2020-05-18T04:37:34Z

perl/lib/Wallet/ACL.pm

    eval {
        my $guard = $self->{schema}->txn_scope_guard;
        my %search = (ae_id => $self->{id});
+        my %options = (order_by => { -asc => [qw/ah_on ah_id/] });


This doesn't look right. These aren't fields in this table.

rra · 2020-05-18T04:45:37Z

perl/lib/Wallet/ACL.pm


        # Add to the history table.
-        my $date = DateTime->from_epoch (epoch => $time);
+        my $date = DateTime->from_epoch (epoch => $time, time_zone => $TZ);


This and all of the other time zone changes look wrong to me. This seems to be putting local times into the database. If the database is timezone-aware, that's okay because it will convert to a canonical time, but if it's not (like SQLite), I believe this will cause all sorts of problems. All times need to be stored as UTC to avoid time zone problems.

I'm not sure what problem you're trying to fix. Maybe there's a missing conversion to a local time zone on display?

rra · 2020-05-18T04:46:05Z

perl/lib/Wallet/ACL.pm

        my $guard = $self->{schema}->txn_scope_guard;
        my %search  = (ah_acl => $self->{id});
-        my %options = (order_by => 'ah_on');
+        my %options = (order_by => { -asc => [qw/ah_on ah_id/] });


This seems obviously correct, so I've cherry-picked this one change.

rra · 2020-05-18T04:47:33Z

perl/lib/Wallet/Admin.pm

+    # Suppress warnings that actually are just informational messages.
+    local $SIG{__WARN__} = sub {
+        my ($warn) = @_;
+        return if $warn =~ m{NOTICE:  table "\S+" does not exist, skipping};
+        warn $warn;
+    };
+


Why is a schema deploy causing warnings? This feels like a bug in DBIx::Class or in how we're using it that should be fixed there rather than trying to match strings in warning messages.

rra · 2020-05-18T04:48:14Z

perl/lib/Wallet/Admin.pm

+
+    # Suppress warnings that actually are just informational messages.
+    local $SIG{__WARN__} = sub {
+        my ($warn) = @_;
+        return if $warn =~ m{NOTICE:  table "\S+" does not exist, skipping};
+        warn $warn;
+    };
+


Same question here.

rra · 2020-05-18T04:56:05Z

perl/t/general/report.t

 is (scalar (@{ $acls[0] }), 2, ' with two members');
 is ($acls[0][0], 'fourth', ' and the first member is correct');
 is ($acls[0][1], 'third', ' and the second member is correct');
-


Stray whitespace change.

rra · 2020-05-18T04:57:06Z

perl/t/general/server.t

+# ### ## ### ## ### ## ### ## ### ## ### ## ### ## ### ## ### #
+sub get_acl_id {
+    my ($acl_name) = @_;
+    my $acl_temp = Wallet::ACL->new ($acl_name, $schema);
+    return $acl_temp->id;
+}
+# ### ## ### ## ### ## ### ## ### ## ### ## ### ## ### ## ### #


Style nit: Could you move this up above "Some global defaults" after the use statements and remove the comment boundaries?

rra · 2020-05-18T04:57:46Z

perl/t/general/server.t

 $server = eval { Wallet::Server->new ($user2, $host) };
 like ($@, qr/unable to open database file/,
      ' or if the database connection fails');
+


Stray whitespace change.

rra · 2020-05-18T04:58:34Z

perl/t/general/server.t

    unlink 'wallet-db';
 }

+local $ENV{DBIC_NO_VERSION_CHECK} = 1;


This looks obviously correct so I cherry-picked it.

rra · 2020-05-18T05:00:47Z

perl/t/lib/Util.pm

+sub setup_initialize {
+    my $admin;
+    eval {
+        local $ENV{DBIC_NO_VERSION_CHECK} = 1;


I don't understand this change or why it's in an eval. Isn't this handled by db_setup_sqlite? And swallowing all exceptions seems wrong.

acl comment and build changes

863dfd9

revert version number

rra reviewed May 18, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add acl comment and some build chagnes#79

Add acl comment and some build chagnes#79
macrotex wants to merge 1 commit intorra:upstream/latestfrom
macrotex:aclcomment

macrotex commented Feb 18, 2020

Uh oh!

rra left a comment

Uh oh!

rra May 18, 2020

Uh oh!

rra May 18, 2020

Uh oh!

rra May 18, 2020

Uh oh!

rra May 18, 2020

Uh oh!

rra May 18, 2020

Uh oh!

rra May 18, 2020

Uh oh!

rra May 18, 2020

Uh oh!

rra May 18, 2020

Uh oh!

rra May 18, 2020

Uh oh!

rra May 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

macrotex commented Feb 18, 2020

Uh oh!

rra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants