Skip to content

Conversation

nagem
Copy link
Contributor

@nagem nagem commented Aug 31, 2017

Closes #903

Site level gear rules serve as a base list of gear rules for all new projects.
Notes:

  • A project created via uploading data or POSTing to /api/projects will have a copy of all site rules assigned to that project. They can be edited and deleted after project creation.
  • Site admin is required to add/modify/delete site-level gear rules. All logged in users can view them.
  • Modifying site-level rules do not modify the project rules created from them. It is a one-time copy at project creation time. There is currently no way to push rules down to projects.

Breaking Changes

None

New Endpoints

Get all site level rules

  • login required
GET https://baseUrl/api/site/rules

Get site level rule by

  • login required
GET https://baseUrl/api/site/rules/<rule-id>

Add new site level rule

  • site admin required
POST https://baseUrl/api/site/rules
{
    "all" : [ 
        {
            "type" : "file.type",
            "value" : "dicom"
        }
    ],
    "name" : "Run dcm2niix on dicom",
    "alg" : "dcm2niix",
    "any" : []
}

Modify site level rule

  • site admin required
PUT https://baseUrl/api/site/rules/<rule-id>
{
    "alg": "modification"
}

Remove site level rule

  • site admin required
DELETE https://baseUrl/api/site/rules/<rule-id>

Review Checklist

  • Tests were added to cover all code changes
  • Documentation was added / updated
  • Code and tests follow standards in CONTRIBUTING.md

@nagem nagem requested review from kofalt and ryansanford August 31, 2017 15:53
@nagem
Copy link
Contributor Author

nagem commented Sep 1, 2017

@dpuccetti

@ryansanford
Copy link
Contributor

@nagem
I'm almost finished with the infra changes. Site level gear rules means I no longer need POSt of project level gear rules to be allowed with an alg that doesn't exist yet on the system, so we could tighten up validation there, unless there's a use case beyond my own I'm not aware of.

The visibility of the special project_id was unexpected. Would like to know reasons for that if intentional.

Otherwise, looks/tests great.

@nagem
Copy link
Contributor Author

nagem commented Sep 1, 2017

They use the same endpoint (project and site), so tightening it up would require a "if project level gear, validate the alg exists" block, right? Because site rules would still need to be able to do that?

I can make a minor change to remove the project_id on the rule when returned from the site routes. I just reused that key so the handler methods would operate the same.

@ryansanford
Copy link
Contributor

@nagem I don't need the ability to post gear rules at either project or site level, where the referenced gear doesn't exist yet. Thus, from my perspective, enforcement at both levels would be fine. I'm not saying we should enforce it. Just that my use case for it being lax no longer applies so enforcement could be re-considered.

@codecov-io
Copy link

codecov-io commented Sep 5, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@92102d7). Click here to learn what that means.
The diff coverage is 98.31%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #921   +/-   ##
=========================================
  Coverage          ?   90.35%           
=========================================
  Files             ?       48           
  Lines             ?     6369           
  Branches          ?        0           
=========================================
  Hits              ?     5755           
  Misses            ?      614           
  Partials          ?        0
Flag Coverage Δ
#python 90.35% <98.31%> (?)
Impacted Files Coverage Δ
api/api.py 100% <ø> (ø)
api/handlers/listhandler.py 90.65% <100%> (ø)
api/dao/base.py 86.42% <100%> (ø)
api/jobs/gears.py 84.37% <100%> (ø)
api/dao/containerstorage.py 88.97% <100%> (ø)
api/jobs/rules.py 86.92% <100%> (ø)
api/dao/hierarchy.py 84.84% <100%> (ø)
api/dao/containerutil.py 86% <96.15%> (ø)
api/jobs/handlers.py 98.81% <98.59%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92102d7...990920b. Read the comment docs.

@nagem
Copy link
Contributor Author

nagem commented Sep 6, 2017

@ryansanford script added.

@ryansanford
Copy link
Contributor

@nagem Thanks for the script. Works as expected.

How would you feel about making the script more generic, with flags to execute various "future" checks. Something that would be executed like... python bin/check_integrity.py --rule-algs

Also, could I get a non-zero exit code when a check finds something wrong? I did confirm that exception, like failure to connect to db results in exit code = 1. 👍

@nagem
Copy link
Contributor Author

nagem commented Sep 6, 2017

@ryansanford I added the script integrity_check.py with two dummy integrity checks so you could test out the format to see if you like it.

usage: check_rule_algs.py [-h] [--all] [--check_rule_alg] [--test_one]
                          [--test_two]

optional arguments:
  -h, --help        show this help message and exit
  --all             Run all checks
  --check_rule_alg  Confirm alg keys in rules table can be resolved to gear in
                    gear table
  --test_one        Run test one
  --test_two        Run test two

I don't do a whole lot of error checking, if you pass --all and other checks, it just runs all of them. passing no arguments does nothing. If you think that is a risk, I can add more input error handling.

@ryansanford
Copy link
Contributor

@nagem Looks nice. Working as expected, except not getting non-zero exit when any of the test fail.

@ryansanford
Copy link
Contributor

LGTM

Changes work great!

@nagem nagem merged commit 816f937 into master Sep 6, 2017
@nagem nagem deleted the site-rules branch September 6, 2017 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants