In pift.cron.inc:
- pift.cron.inc documentation needs updating (all branch 'client_identifier' references)
- pift_cron_queue_batch_build_branches
- Pulls id as rid from pifr_test.
- id needs to be updated to label nodes
- pift_cron_queue_batch_build_branches_process Line 255 on
- loads the release nid and populates branch information
- Need to refactor to support if no release nid.
- Need to refactor to set the label id instead of nid as client_identifier

- Pift.cron.inc Line 299
- Loads the $item['dependency'] array for drupal core
- May need to load the Label ID instead
- Line 303 loads project_info_dependency_list_get($branch->nid)
- Once project_dependency is in place, this code will need to be updated to use label ids when it sets $item['dependency'].
- Currently not used.

PIFR:
If we simply start using label ids, branch Identifiers won't exist, so they become new entries in pifr_server_branch_save.
- To avoid duplicates in the branch table, need to convert all branch release nodes to their vcs_label id equivalents in associated database tables

Branch client_identifiers are used in pifr_server.test, so would need to be updated.

Used in pifr_shell_input_example_get('pifr_queue') and queue.example.php.

Branch Identifiers are used in branch dependencies ... need to update them too.
- Line 249 of pifr_server.test.inc (pifr_server_test_xmlrpc) queries by $dependency
- Can be left if we convert all branch dependencies to label ids

- Possible dependency issues in server.inc (argument() on line 266)?
- called from pifr_server.review.inc (review_argument_get() on line 14)?

Comments

jthorson’s picture

Using this issue to track my testing for this issue ...

Backup pift-test table.

CREATE TABLE pift_test_original LIKE pift_test;
INSERT pift_test_original SELECT * from pift_test;

To Restore:

DROP TABLE pift_test
CREATE TABLE pift_test LIKE pift_test_original;
INSERT pift_testSELECT * from pift_test_original;

Update the id value for all branch tests in Pift_Test table to the label value.
- We can use the versioncontrol_release_labels table to convert 'release_nid' to 'label_id'.

1. Find all branches in test table which do not have corresponding release nids. Assuming there's not too many, these will need to be cleaned up manually.
Branch IDs:

select id from pift_test where type=1 and id not in (Select release_nid from versioncontrol_release_labels);

Full Tests:

Select * from pift_test where id IN (select id from pift_test where type=1 and id not in (Select release_nid from versioncontrol_release_labels));

Or just delete them, since they don't have matching branch ids:

Delete from pift_test where id IN (select id from pift_test where type=1 and id not in (Select release_nid from versioncontrol_release_labels));

2. Select which gives you the updated table:

Select t.test_id as test_id, t.type as type, l.label_id as id, t.status as status, t.message as message, t.last_tested as last_tested from pift_test t, versioncontrol_release_labels l where t.id = l.release_nid and t.type = 1 order by t.id;

3. Perform the update:

update pift_test t, versioncontrol_release_labels l SET t.id=l.label_id where t.type=1 and t.id=l.release_nid;

Update Pift Code:
1. Line 205:
- branch_info_result queries from project release nodes. To support patch testing in sandboxes, we need to update this to use label ids.
- Since sandbox issues don't have a 'version' set, we can't key off of project_issue rid, like the original query does. In fact, we don't have anything to link us to a specific release. Need to first check if a release exists, and is set ... and if not, don't queue the file.
- TODO!

2. pift_cron_queue_batch_build_batches_process()
- Changes done on local machine.
- Doesn't set drupal.modules correctly (need branch nid). Should be fixed by project_dependency
- Doesn't set contrib dependencies correctly, but this was already broken.

to be cont'd ...

jthorson’s picture

First attempt at changes for pift.cron.inc.

rfay’s picture

Subscribe

jthorson’s picture

PIFR_Server:

We don't have a reference to the label_ids in the qa database, so we need to get them from the d.o side. For now, just copy the versioncontrol_release_labels table.

1. Export the versioncontrol_release_labels table
drush sql-dump --tables-key=versioncontrol_release_labels --result-file vcs_label.sql

2. Copy 'vcs_label.sql' file over to qa.d.o and import into database
d.o:
mv vcs_label.sql vcs_label.sql.txt
qa.d.o:

wget http://drupal.org/vcs_label.sql.txt
mv vcs_label.sql.txt vcs_label.sql
`drush sql-connect` < vcs_label.sql

3. Backup pifr_branch table on qa.d.o

CREATE TABLE pifr_branch_original LIKE pifr_branch;
INSERT pifr_branch_original SELECT * from pifr_branch;

To Restore:

DROP TABLE pifr_branch
CREATE TABLE pifr_branch LIKE pifr_branch_original;
INSERT pifr_branch SELECT * from pifr_branch_original;

4. Update references in pifr_branch table on qa.d.o
Determine which records will not be updated - These will have to be manually fixed (or deleted).

select client_identifier from pifr_branch where client_identifier not in (Select release_nid from versioncontrol_release_labels);

Update records

update pifr_branch t, versioncontrol_release_labels l SET t.client_identifier=l.label_id where t.client_identifier=l.release_nid;
jthorson’s picture

- PIFT patch has a bad watchdog line (missing array() argument on line 292)

- Requeueing a test results in a watchdog error:
Invalid release ID [208375] for test ID [].
This is because the query at line 277 is using a release nid value for $rid, instead of the new label_id value. Due to the issue documented at line 205 of pift.cron.inc.

- Issue with drupal.modules code:
Notice: Trying to get property of non-object in pift_cron_queue_batch_build_branches_process() (line 401 of /var/www/dev/rfay.redesign.devdrupal.org/htdocs/sites/all/modules/project_issue_file_test/pift.cron.inc).
I originally was going to leave this alone due to the project_dependencies work, but it causes problems as is.

- Branch test fails validation due to invalid dependencies.
Need to update the queue validation PIFR code.

jthorson’s picture

Updated pift.cron.inc patch.

jthorson’s picture

PIFR dependency errors are due to the PIFT module not including dependencies properly.

Original test query:

Array
(
    [branches] => Array
        (
            [0] => Array
                (
                    [project_identifier] => 106016
                    [client_identifier] => 734592
                    [vcs_identifier] => 7.x-1.x
                    [dependency] => 156281
                    [plugin_argument] => Array
                        (
                            [drupal.modules] => Array
                                (
                                    [0] => token_test
                                    [1] => token
                                )

                        )

                    [test] => 1
                    [link] => http://rfay.redesign.devdrupal.org/node/734592
                )

            [1] => Array
                (
                    [project_identifier] => 3060
                    [client_identifier] => 156281
                    [vcs_identifier] => 7.x
                    [dependency] => 
                    [plugin_argument] => Array
                        (
                            [drupal.core.version] => 7
                        )

                    [test] => 
                    [link] => http://rfay.redesign.devdrupal.org/node/156281
                )

        )

    [files] => Array
        (
        )

    [projects] => Array
        (
            [0] => Array
                (
                    [client_identifier] => 106016
                    [name] => Token
                    [repository_type] => git
                    [repository_url] => git://git.drupal.org/project/token.git
                    [link] => http://rfay.redesign.devdrupal.org/project/token
                )

            [1] => Array
                (
                    [client_identifier] => 3060
                    [name] => Drupal core
                    [repository_type] => git
                    [repository_url] => git://git.drupal.org/project/drupal.git
                    [link] => http://rfay.redesign.devdrupal.org/project/drupal
                )

        )

)

Post-patch code (missing second array element for both main and dependencies):

Array
(
    [branches] => Array
        (
            [0] => Array
                (
                    [project_identifier] => 106016
                    [client_identifier] => 106690
                    [vcs_identifier] => 7.x-1.x
                    [dependency] => 60
                    [plugin_argument] => Array
                        (
                        )

                    [test] => 1
                    [link] => http://rfay.redesign.devdrupal.org/node/734592
                )

        )

    [files] => Array
        (
        )

    [projects] => Array
        (
            [0] => Array
                (
                    [client_identifier] => 106016
                    [name] => Token
                    [repository_type] => git
                    [repository_url] => git://git.drupal.org/project/token.git
                    [link] => http://rfay.redesign.devdrupal.org/project/token
                )

        )

)
jthorson’s picture

Testing out a hunch ... new pift patch attached.

jthorson’s picture

http://qa.scratch.drupal.org/pifr/test/80809 looks good with shortcut enabled ... but Token 7.x-1.x is running a full test on scratchtestbot (due to the 'drupal.modules' and 'test.directory.review' arguments not being datafilled).

May indicate a flaw with the is_core_test() code ... but should be close enough to work into the project_dependency implementation.

webchick’s picture

Tagging.

jthorson’s picture

New approach:

Database changes
1. Add a new 'nid' column to pift_test.
2. Update pift_test, datafilling the 'issue nid' for file tests, and copying the 'release nid' over from the 'id' column for branch tests.
3. Update the 'id' column with label_ids from versioncontrol_labels for all branch tests.

Code changes
1. When writing a new file test to pift_test, include the issue nid.
2. When writing a new branch test to pift_test, include the label_id in id, and release_nid in nid (if one exists)
3. When creating a 'branch' batch, check if the branch 'nid' is datafilled.

  • If so, use that as the 'client identifier' for the batch.
  • If not, check if the label has a release node.
    • If so:
      • update the 'nid' column
      • set $batch['branch'][x]['update_id'] = $label_id
      • use 'nid' as the client identifier
    • otherwise, use 'id' as the 'client identifier' for the batch.

Pros:
1. No db manipulation on PIFR required
2. Should make #675460: Ensure/Refactor cron_retest() query to only re-test the last file on an issue; Automatically retest RTBC patches trivial. (+1!)

Cons:
1. Would result in duplicate branch test records on qa.d.o, one from the pre-release test, and a new entry after the official release.

  • This is solved with a slight tweak to PIFR, having it key off the $batch['branch'][x]['update_id'] value in the $batch array, and updating the value in it's own pifr_branch table.
boombatower’s picture

Seems like a good plan, we talked in IRC as well.

jthorson’s picture

Title: Conversion notes, impacts, and strategies » Conversion impacts the following

Problem: Sandbox issue queues do not have an associated 'version' drop-down.

Proposal:
1. When building $batch['files'], attempt to parse out a version string from the file name (e.g. mypatchname-6.x-1.x.patch). If successful, determine the associated release nid (if one exists) or label_id (if no release_nid) for that given branch/tag name.
2. If unable to parse a version from the file name, then attempt to use the current version from the issue itself.
3. If the issue does not have an associated current version (i.e. sandbox issues do not), then see if the project has only one potential branch.
4. If the version can not be resolved, then the patch should be marked as 'ignored' (preferrably with a descriptive message) and dropped from the batch.

Added Bonus:
- Would also enable testing of "backport" patches in the same issue/comment as the original, without needing to toggle the issue version.

jthorson’s picture

Title: Conversion impacts the following » Conversion notes, impacts, and strategies
rfay’s picture

Title: Conversion impacts the following » Conversion notes, impacts, and strategies

First, I think it's always been a mistake when we tried to infer things from patch file names. I don't really go for that.

On determining the version I'm baffled by this approach. (#2 and #3 in #13 ). You can't have a version in the issue if there are no releases, because that's where versions come from.

How about parsing the version from the info file?

boombatower’s picture

@rfay: I think the problem is two fold and there are two "versions" referred to here. 1) The Drupal core API compatibility, 2) the "version" or branch the issue is associated with.

#1 can be solved by reading the .info file, but that assumes you know which branch to read from which is the problem @jthorson is attempting to solve. The number or problems that arrise due to not having releases in sandboxes just grows and grows...seems to be that we either need sandboxes to grow up and get dev releases or project_issue to support associating an issue with a branch or tag...which seems foolish since it already does that with releases so we are back to letting sandboxes have some sort of releases...or FAKE releases...something. This just ends up re-creating half of the project module.

Only technical pitfall I heard against sandbox releases was two sandboxes having the same namespace, but something tells me we can deal with that MUCH MUCH easier rewriting half of the project module.

rfay’s picture

releases++

Or even releases without packaging.

webchick’s picture

The security team has shot down dev releases on sandboxes (twice now), so that's a no-go, at least in terms of something downloadable by end users. "Fake" releases might be possible, but it's unclear how that would work.

jthorson’s picture

rfay (Re: 15):

On determining the version I'm baffled by this approach. (#2 and #3 in #13 ). You can't have a version in the issue if there are no releases, because that's where versions come from.

The confusion may be because I'm using 'version' in the generic sense in the first half of the item, and in the 'project' sense in the second half. This could be rewritten ...

1. When building $batch['files'], attempt to parse out a branch/tag string from the file name (e.g. mypatchname-6.x-1.x.patch). If successful, determine the associated release nid (if one exists) or label_id (if no release_nid) for that given branch/tag name.
2. If unable to parse a branch/tag from the file name, then attempt to use the branch/tag that matches the current release version from the issue itself.
3. If the issue does not have an associated current release version (i.e. sandbox issues do not), then see if the project has only one potential branch.
4. If the branch/tag can not be resolved, then the patch should be marked as 'ignored' (preferrably with a descriptive message) and dropped from the batch.

boombatower’s picture

Yea, by fake I mean they can never show up on project page and not have packaging...but just having the data entry makes so much sense....it just breaks too much...even things outside of pift.

jthorson’s picture

After discussing with dww at BADCamp, here is the latest (please, can I say 'final' yet?) approach:

Problem: Sandbox issue queues do not have an associated 'version' drop-down.

Proposal:
1. Refactor Project_Issue to provide a 'version string' dropdown select box instead of 'project release' select box on the 'project issue' form.
2. Add the 'version string' select box to sandbox projects as well as the existing 'full' projects.
3. Where required, perform a lookup on versioncontrol_release_labels and/or versioncontrol_labels to determine the release_nid or label_id referenced by a given version string.

Unfortunately, this loses the bonus of free backport tests ... unless we do BOTH approaches, which could have some merit.

jthorson’s picture

Just parking this patch here, in case I need any of the code during the test sprint.