fixes bug 785430 - Add a service to add new products

[peterbe]

@rhelmer r?

This basically just does what the documentation says to do except it makes it possible to kick this off from the middleware.

Once this has been done and you think it's good, I'll add it to the webapp and its admin UI.

[rhelmer]

So one thing I'd suggest is not hardcoding the build ID, but generate it based on the current date - the underlying stored procedure ignores a build ID older than 30 days.

I'd also suggest that the test should go a little further and actually check that the tables are populated as expected... let me play with this patch and I'll give you some concrete examples, in the form of a patch or PR)

[rhelmer]

I would like to see this service refuse to accept a new product or release that we know won't work with our current restrictions - things like the build ID issue that I mentioned, and also with things like the product name (no spaces) and version number (x.y with optional a(n)/b(n), e.g. "3.0", "3.0a1", "3.0b1"), channel name must be one of Release/Beta/Aurora/Nightly... we shouldn't make it possible to add new products/releases that we know will simply not work, and if you violate these rules Socorro will break in hard-to-debug ways.

Now, these limitations are purely arbitrary and should be lifted (especially things like channel names and version string formats), but in the meantime I think it'd be way more helpful to people if we document and enforce these... we can adjust the enforcement as we adjust the underlying schema.

[adngdb]

Why did you put this in a new file and not in external/potsgresql/products.py? You can do that and call your function post_releases or create_releases, and it will work the same. The original thought was to organize endpoints per resource (here, the products) and then have a function per speciality.

[peterbe]

I did so to not muddle things. This new file is about products AND releases. That's why I didn't think it belongs in products.py which is all about products.
Also, creating new files is cheap and is keeps the individual LOC per file shorter and easier to keep things apart.
Lastly, we, mozilla, won't actually be using this new service since we get our products and releases from the ftpscraper so by making this little thing its own file it felt like it made less of a complication-impact.

What do you think? Did I really commit a sin or am I making some sense?

[peterbe]

@rhelmer I change the build_id to be auto-generated. I haven't added any more validation.
Thing is, normally we do most of our validation higher up as that gives a better chance to report back to the user why their input is wrong.
The few validation rules we have are quite brutal things like return a 400 Bad Request if say the start date is too far away from the end date.

Either way, we can add some basic validation rules if you think it's necessary. Like:

1. Disallow spaces in the product_name
2. Disallow release_channel inputs that isn't present in the release_channels table.
3. Some magic on the version number. (which I'm not confident I get from your description)

I'm happy to help about writing this in code, but I'm eager to hear what you think about adding this validation in the webapp instead. Or both.

[rhelmer]

@peterbe I had thought about punting validation to the web app, but I think it makes sense for the middleware to not allow something that we know won't work, and it has access to the DB.

The 3 basic validation rules you describe are exactly what I was thinking of... #3 should be something like: "an integer at least 10 digits long that can be parsed into a date string no more than 30 days old".

[rhelmer]

Hmm playing around with this locally, I wonder if we could push more of the validation to the DB schema (which is probably where it belongs)... for example release_channel is already checked:

16:44:01 middleware.1 | DETAIL:  Key (release_channel)=(blah) is not present in table "release_channels".

[rhelmer]

@peterbe I can't seem to add more than one release - it tries to add a new product every time:

16:46:22 middleware.1 | DETAIL:  Key (product_name, release_channel)=(KillerApp, Release) already exists.

Should adding a product be a separate API call from adding a release?

[rhelmer]

platform could be checked against os_names table too

[rhelmer]

Also in case it's helpful ;) I finally managed to get an invocation that does work fine, on a fresh DB:

curl -X POST -F 'product=KillerApp' -F 'version=5.0a1' -F 'update_channel=Nightly' -F 'build_id=201408010000' -F 'platform=Linux' -F 'beta_number=0' -F 'release_channel=Nightly' -F 'throttle=0' 'http://localhost:5200/products/releases/'

http://localhost:5200/products/ now contains:

    "products": [
        "B2G",
        "WaterWolf",
        "NightTrain",
        "KillerApp"
    ]

Looking at it in the UI - it does work, but there are no "featured" versions so the UI is a little wonky (graph shows an error by default)

[peterbe]

Change this not insert if it already exists on product + release_channel

[peterbe]

Note-to-self: Take Adrian's suggested and split this up into two create() methods in products.py and releases.py. Only call the update_product_versions() after adding the new release.
Also, the insert on the product_release_channels should only happen if we don't have the product + release_channel before.

[peterbe]

closing it because it needs a lot of work by me.

[peterbe]

@rhelmer That was almost too easy.
I've exploded the products_releases.py across products.py and releases.py.

[adngdb]

If any of those fields is missing, it will raise an error that won't be caught. Wouldn't it be better to use external_common.parse_arguments in this function as well, and handle those mandatory parameters in a more robust way (by raising a MissingArgumentError for example)?

[peterbe]

Go back to your vacation!!!

But yeah, you're right. I'll amend that.

[adngdb]

/me is much happier with the file organization. :-)

[rhelmer]

This looks great! I like the separate services ... as we discussed let's do the sanity-checking in the SPs so you don't have to deal with it here.

I'll test this out locally.

[rhelmer]

Still on my list, just had some higher-priority goal stuff take precedence.

[rhelmer]

r+ this works for me - it's really hard to guess what format everything needs to be in for reports to actually work etc. We can tackle this in the SPs though as discussed