Bug 1409957 - Create polling daemon to query Phabricator for recent transcations and update bug data according to revision changes - #264
Merged
Conversation
…ent transcations and update bug data according to revision changes
darkwing
reviewed
Oct 19, 2017
| next; | ||
| } | ||
| my ($revision) = get_revisions_by_phids([$object_phid]); | ||
| $self->logger->info("REVSION: " . $revision->{'id'} . ": " . |
|
Small Perl observation here: "REVISION: $revision->{'id'}: " would work
fine.
…On Thu, Oct 19, 2017 at 11:48 AM, David Walsh ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In extensions/PhabBugz/lib/Feed.pm
<#264 (comment)>:
> + $self->logger->info("FEED: No new transactions");
+ return;
+ }
+
+ # Process each story
+ foreach my $story (keys %$transactions) {
+ $self->logger->info("STORY: $story");
+ my $story_data = $transactions->{$story};
+ my $object_phid = $story_data->{objectPHID};
+ $self->logger->info("OBJECT: $object_phid");
+ if ($object_phid !~ /^PHID-DREV/) {
+ $self->logger->info("SKIP: Not a revision change");
+ next;
+ }
+ my ($revision) = get_revisions_by_phids([$object_phid]);
+ $self->logger->info("REVSION: " . $revision->{'id'} . ": " .
REVSION typo
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#264 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFqDAEPGJIF0SH8aAH2A9OMuJH3S-Bjks5st5mUgaJpZM4P-qgV>
.
|
Contributor
|
Adding @dylanwh as a reviewer as I'm not nearly qualified to approve this large a change. |
dylanwh
suggested changes
Oct 23, 2017
|
|
||
| our $VERSION = '0.01'; | ||
|
|
||
| BEGIN { |
Contributor
There was a problem hiding this comment.
I'd rather see you just call Bugzilla::Extensions::PhabBugz->get_instance() (remove the _). That way the person reading the code will know which file to look in.
| } | ||
|
|
||
| sub _get_instance { | ||
| my $cache = Bugzilla->request_cache; |
Contributor
There was a problem hiding this comment.
You're storing this in the request cache, but the daemon isn't ever clearing the request cache. What is the lifecycle of this object intended to be in the daemon? Always there?
| my $self = shift; | ||
| my $pidfile = $self->{gd_args}{pidfile}; | ||
| if (!$pidfile) { | ||
| $pidfile = bz_locations()->{datadir} . '/' . $self->{gd_progname} . ".pid"; |
Contributor
There was a problem hiding this comment.
$pidfile = File::Spec->catfile(bz_locations()->{datadir} , self->{gd_progname} . ".pid");is more idiomatic, though I bet we're not using that elsewhere.
|
|
||
| sub start { | ||
| my ($self) = @_; | ||
| while (1) { |
Contributor
There was a problem hiding this comment.
optional
It might be better to loop in the code that calls this, and also look into https://metacpan.org/pod/Daemon::Generic::While1.
required
Meanwhile if this daemon does anything with bugs, it will eventually run out of memory. You at least need to call Bugzilla->_cleanup() at the end of every run of the loop.
| ThrowUserError('invalid_phabricator_revision_id') | ||
| unless (exists $result->{result}{data} && @{ $result->{result}{data} }); | ||
|
|
||
| return @{$result->{result}{data}}; |
Contributor
There was a problem hiding this comment.
if this is potentially large, it might be best to return the reference and reference it at the place it is used. Otherwise it will do a copy of the full array.
…ent transcations and update bug data according to revision changes
New Revision.pm class that will eventually handle all loading and updating of a revision How to test: 1. You need to be running a mozilla/phabext image that contains the latest phabricator-extensions code that has the feed.query_epoch Conduit API call. 2. cd /path/to/mozilla-conduit/bmo-extensions 3. docker-compose up -d --build 4. docker exec -it bmoextensions_bmo.test_1 su - bugzilla 5. cd /var/www/html/bmo 6. perl extensions/PhabBugz/bin/phabbugz_feed.pl -f -d start 7. In Phabricator, create a new revision with the bug id of an existing bug in the bugzilla database. 8. BMO should update the revision with either public or private policies depending on the bugs permissions.
darkwing
previously requested changes
Nov 1, 2017
| return @{$result->{result}{data}}; | ||
| } | ||
|
|
||
| <<<<<<< HEAD |
Contributor
There was a problem hiding this comment.
This seems to be a problem.
Contributor
|
Note: I had to run |
Contributor
|
Wow, so I stumbled upon a recurring error and I think it's an edge case with my machine but could become a real issue. In my auth testing, I would delete users but their old revisions would still be there, so it's possible feed items will have users that have been deleted and thus have no BMO ID. Here's the error I get each time I try to run the script as told to: Also, each successive attempt tries to execute another transaction; is it possible the failures aren't keeping things in order? |
loading existing projects and creating new ones. - More cleanup and bug fixes.
…into phabbugz-feed * 'phabbugz-feed' of https://github.com/mozilla-bteam/bmo: Fixed merge issue with PhabBugz/lib/Util.pm
Contributor
|
With new updates still getting this error: |
Contributor
|
Here's the payload I'm receiving: https://gist.github.com/darkwing/bd161c535065d8c090a136377f3eb452 |
darkwing
reviewed
Nov 6, 2017
| } | ||
| }; | ||
|
|
||
| if ($params->{ids}) { |
Contributor
There was a problem hiding this comment.
Can we add a comment here as to why ids vs. phids vs. {}? Can you describe the cases here? Is it story types? Would help for maintenance.
- Refactored where phab_user is set and where we wrap in a DB transaction - More debugging output that only displays when using -d - Some bug fixes
darkwing
reviewed
Nov 6, 2017
|
|
||
| # Skip changes done by phab-bot user | ||
| my $phab_users = get_phab_bmo_ids({ phids => [$author_phid] }); | ||
| if (@$phab_users) { |
Contributor
There was a problem hiding this comment.
Do we want to avoid this if() if $skip is already flagged?
There was a problem hiding this comment.
If get_phab_bmo_ids is expensive, then yes for certain.
- Fixes for Feed.pm to prevent looping - Added code to update review flag statuses when revision is accepted/rejected.
Collaborator
Author
|
How to test:
Optional: Due to code being in the Doorkeeper extension in Phabricator, you will probably see double changes being made to revisions. To stop this, you can do the following:
|
|
Note that Puppet typically runs twice an hour, which usually conflicts with
SLAs.
…On Thu, Nov 9, 2017 at 09:59 Dylan William Hardison < ***@***.***> wrote:
So if phabricator is down, the daemon will die. Do our daemons normally
recover from that?
e.g. will puppet or something notice that the daemon has died and restart
it?
Reason: Can't connect to 192.168.3.1:5000
at /vagrant/extensions/PhabBugz/lib/Util.pm line 430.
Bugzilla::Extension::PhabBugz::Util::request(...) called at /vagrant/extensions/PhabBugz/lib/Feed.pm line 294
Bugzilla::Extension::PhabBugz::Feed::feed_transactions(...) called at /vagrant/extensions/PhabBugz/lib/Feed.pm line 64
Bugzilla::Extension::PhabBugz::Feed::feed_query(...) called at /vagrant/extensions/PhabBugz/lib/Feed.pm line 38
Bugzilla::Extension::PhabBugz::Feed::start(...) called at /vagrant/extensions/PhabBugz/lib/Daemon.pm line 96
Bugzilla::Extension::PhabBugz::Daemon::gd_run(...) called at local/lib/perl5/Daemon/Generic.pm line 192
Daemon::Generic::new(...) called at local/lib/perl5/Daemon/Generic.pm line 37
Daemon::Generic::newdaemon(...) called at /vagrant/extensions/PhabBugz/lib/Daemon.pm line 24
Bugzilla::Extension::PhabBugz::Daemon::start(...) called at extensions/PhabBugz/bin/phabbugz_feed.pl line 22
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#264 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFqDPWDfSBebQQqjAEuxovnGMUADdVPks5s0z1lgaJpZM4P-qgV>
.
|
Contributor
|
okay, so there should be an eval the catches fatal errors and logs them. |
|
If IT-hosted, we also offer daemontools.
…On Thu, Nov 9, 2017 at 10:02 Dylan William Hardison < ***@***.***> wrote:
okay, so there should be an eval the catches fatal errors and logs them.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#264 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFqDKHs7n8_lkRNxihMnSen4Hl2_wrxks5s0z4xgaJpZM4P-qgV>
.
|
dylanwh
reviewed
Nov 9, 2017
dylanwh
left a comment
Contributor
There was a problem hiding this comment.
Okay, so we're cool that we iterate over the stories in a random order?
dylanwh
reviewed
Nov 9, 2017
| } | ||
|
|
||
| # Process each story | ||
| foreach my $story (keys %$transactions) { |
Contributor
There was a problem hiding this comment.
This will iterate over the stories in a random order.
|
Would an ORDER BY clause be sufficient or are there additional perl reasons
beyond that?
…On Thu, Nov 9, 2017 at 10:17 Dylan William Hardison < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In extensions/PhabBugz/lib/Feed.pm
<#264 (comment)>:
> + $self->logger->info("FEED: Fetching new transactions");
+
+ my $last_ts = $dbh->selectrow_array("
+ SELECT value FROM phabbugz WHERE name = 'feed_last_ts'");
+ $last_ts ||= 0;
+ $self->logger->debug("QUERY LAST_TS: $last_ts");
+
+ # Check for new transctions (stories)
+ my $transactions = $self->feed_transactions($last_ts);
+ if (!%$transactions) {
+ $self->logger->info("FEED: No new transactions");
+ return;
+ }
+
+ # Process each story
+ foreach my $story (keys %$transactions) {
This will iterate over the stories in a random order.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#264 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFqDHOwE4ZT6qZiSdFX4Ddxx_qjPBmWks5s00GsgaJpZM4P-qgV>
.
|
Contributor
|
@floatingatoll I don't know, I want to see how feed.query_epoch is defined. |
Contributor
|
I would assert that, for human-related purposes, it is not; debugging and
diagnostic and logging and auditing will all be simpler if the stories are
processed in the same order each time. For example, if the code fails in an
action-at-a-distance manner, it would affect random tasks on every run,
worsening reproducibility.
…On Thu, Nov 9, 2017 at 10:58 AM, Dylan William Hardison < ***@***.***> wrote:
PHP arrays are ordered. The question is: is it okay to process the stories
in a transaction in an arbitrary, random order?
@dklawren <https://github.com/dklawren> @darkwing
<https://github.com/darkwing> ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#264 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFqDIfnnsegi4Z6M6ptFrQ0LBIwLW8rks5s00s8gaJpZM4P-qgV>
.
|
Contributor
|
Fixed the exception handling in #269 |
Contributor
|
Up until the data is turned into json, we get the ordering as returned by $query->execute() on the PHP side. I guess PhabricatorFeedQuery() would discuss that ordering? Or perhaps ordering is totally unimportant. |
Contributor
dylanwh
suggested changes
Nov 9, 2017
| $story_text); | ||
| $self->logger->info($log_message); | ||
|
|
||
| my $bug = Bugzilla::Bug->new($revision->bug_id); |
Contributor
There was a problem hiding this comment.
It is possible for phabricator to initiate a writable SQL injection attack against BMO.
Two things -- the {fields} returned from phabricator should be validated, and the long form ->new({id => $revison->bug_id, cache => 1 }) should be used here
Contributor
|
For the changes related to Revision.pm (only!) I have split off this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1415993, |
…ring and better error checking for API calls. - Update update_project_members.pl to use Project.pm properly. TODO: - Use memcache inside get_phab_bmo_id() calls as the values are unlikely to change - Update the Phabricator.pm Push connector to use Revision.pm
Contributor
|
Contributor
|
So $bug->id is not defined, as $bug is the "error bug" object. |
Contributor
|
Sometimes the feed response is: {"error_info":"Constraint \"todump\" is not a valid constraint for this query.","error_code":"ERR-CONDUIT-CORE","result":null}and other times it is {
"error_info" : null,
"error_code" : null,
"result" : {
"cursor" : {
"after" : null,
"order" : null,
"limit" : 100,
"before" : null
},
"query" : {
"queryKey" : "all"
},
"maps" : {},
"data" : [
{
"attachments" : {
"reviewers" : {
"reviewers" : []
},
"projects" : {
"projectPHIDs" : []
},
"subscribers" : {
"viewerIsSubscribed" : false,
"subscriberPHIDs" : [
"PHID-USER-23ia7vewbjgcqahewncu"
],
"subscriberCount" : 1
}
},
"fields" : {
"bugzilla.bug-id" : "2",
"dateModified" : 1500403974,
"status" : {
"closed" : false,
"value" : "needs-review",
"name" : "Needs Review",
"color.ansi" : "magenta"
},
"policy" : {
"view" : "PHID-PLCY-5x6xprxaxhvghmddosqc",
"edit" : "PHID-PLCY-5x6xprxaxhvghmddosqc"
},
"authorPHID" : "PHID-USER-23ia7vewbjgcqahewncu",
"title" : "test",
"dateCreated" : 1500403552
},
"phid" : "PHID-DREV-nltdtsi6x2cy2gfpbgam",
"type" : "DREV",
"id" : 1
}
]
}
}The first one is a bit confusing. The second one is wrong. Where is bug-id of 2 coming from? |
Collaborator
Author
|
{"error_info":"Constraint "todump" is not a valid constraint for this query.","error_code":"ERR-CONDUIT-CORE","result":null} I have not seen this personally. I do not see in the code where I try to set a 'todump' constraint. Seems a glitch in the API somewhere. As for the bug #2 issue, as we discussed, it seems that the Phabricator DB dump that is part of the bmo-extensions repo needs to be fixed to remove the previously created revision that has 2 as the bug id. Or change generate_conduit_data.pl to create a #2 bug correctly. Not an issue for production systems. |
- Store last ID in the BMO table instead of epoch and use ID for retrieving newest stories. - Updates to different functons in Util.pm to use newer methods for getting data from phabricator. - Moved API error checking into Util::request
dylanwh
approved these changes
Nov 28, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.