| *** wendar_ has joined #storyboard | 05:39 | |
| *** wendar has quit IRC | 05:40 | |
| *** mrmartin has joined #storyboard | 06:07 | |
| *** jtomasek has joined #storyboard | 07:14 | |
| *** jcoufal has joined #storyboard | 07:23 | |
| *** alexismonville has joined #storyboard | 07:26 | |
| *** alexismonville has quit IRC | 07:35 | |
| *** coolsvap|afk is now known as coolsvap | 07:37 | |
| *** alexismonville has joined #storyboard | 07:38 | |
| *** jeblair has quit IRC | 08:09 | |
| *** jeblair has joined #storyboard | 08:11 | |
| *** alexismonville has quit IRC | 08:15 | |
| *** alexismonville1 has joined #storyboard | 08:15 | |
| *** mrmartin has quit IRC | 08:41 | |
| *** MaxV has joined #storyboard | 08:49 | |
| *** CTtpollard has joined #storyboard | 09:12 | |
| *** Piet has quit IRC | 09:29 | |
| *** ssam2 has joined #storyboard | 09:40 | |
| *** mase_x200 has joined #storyboard | 10:09 | |
| *** mase_x200 has quit IRC | 10:13 | |
| *** jedimike has joined #storyboard | 10:15 | |
| *** openstackgerrit has quit IRC | 10:50 | |
| *** openstackgerrit has joined #storyboard | 10:50 | |
| *** mrmartin has joined #storyboard | 12:00 | |
| openstackgerrit | Aleksey Ripinen proposed openstack-infra/storyboard: Added branches to storyboard https://review.openstack.org/150447 | 12:02 |
|---|---|---|
| *** mase_x200 has joined #storyboard | 12:04 | |
| yolanda | here, spec for integration tests: https://review.openstack.org/150743 | 12:21 |
| yolanda | happy to get feedback! as much as possible :) | 12:21 |
| openstackgerrit | Aleksey Ripinen proposed openstack-infra/storyboard: Added branches to storyboard https://review.openstack.org/150447 | 12:51 |
| *** CTtpollard has quit IRC | 13:20 | |
| *** CTtpollard has joined #storyboard | 13:22 | |
| openstackgerrit | Aleksey Ripinen proposed openstack-infra/storyboard: Added branches to storyboard https://review.openstack.org/150447 | 13:27 |
| *** mase_x200 has quit IRC | 13:35 | |
| *** mase_x200 has joined #storyboard | 13:39 | |
| yolanda | NikitaKonovalov, i saw your comments, i think it needs to be fixed in both areas | 13:44 |
| yolanda | first, if the backend is sending date_created, date_updated, it needs to accept that if they come back | 13:44 |
| yolanda | and in the frontend, we need to ensure that we don't send crap, of course | 13:44 |
| NikitaKonovalov | yolanda: so if a client is sending create_at=null or created_at=<something_wrong>, what should a server do? | 13:49 |
| NikitaKonovalov | and if it is sending created_at=<time_that_it_has_from_a_server> what is the reason to send it back to the server? | 13:51 |
| yolanda | client is just sending what it gets from server, so it gets a data that it supposes that it's from the model, and just sent it back untouched | 13:52 |
| yolanda | in our case, frontend is not sending only the updated fields, but all | 13:52 |
| *** mase_x200 has quit IRC | 13:53 | |
| yolanda | i think that accepting them from the backend is fine, because it sends it, but not update that, because they don't belong to the model really | 13:53 |
| yolanda | generally i think that everything that is sent from the server, should be accepted back, but not using that if not declared in the model | 13:54 |
| yolanda | a different thing should be if the frontend sends a new field, not declared in any way or not expected, this should cause an erro | 13:54 |
| *** CTtpollard has quit IRC | 13:57 | |
| *** CTtpollard has joined #storyboard | 13:58 | |
| openstackgerrit | Aleksey Ripinen proposed openstack-infra/storyboard: Added branches to storyboard https://review.openstack.org/150447 | 14:08 |
| *** mattfarina has joined #storyboard | 14:21 | |
| *** MaxV has quit IRC | 15:03 | |
| *** jcoufal_ has joined #storyboard | 15:27 | |
| *** jcoufal has quit IRC | 15:30 | |
| *** MaxV has joined #storyboard | 15:31 | |
| *** MaxV has quit IRC | 15:43 | |
| *** MaxV has joined #storyboard | 15:43 | |
| *** reed has joined #storyboard | 16:06 | |
| rcarrillocruz | folks | 16:39 |
| rcarrillocruz | you getting this exception when running storyboard-worker-daemon: | 16:40 |
| rcarrillocruz | "ArgsAlreadyParsedError: arguments already parsed: cannot register CLI option" | 16:40 |
| rcarrillocruz | wanted to double-check before opening a story | 16:40 |
| *** coolsvap is now known as coolsvap|afk | 16:45 | |
| *** jtomasek has quit IRC | 16:56 | |
| mrmartin | re | 17:00 |
| *** yolanda has quit IRC | 17:03 | |
| *** yolanda has joined #storyboard | 17:04 | |
| *** yolanda has quit IRC | 17:04 | |
| *** yolanda has joined #storyboard | 17:05 | |
| *** wendar_ is now known as wendar | 17:09 | |
| *** MaxV has quit IRC | 17:46 | |
| *** MaxV has joined #storyboard | 17:47 | |
| yolanda | hi rcarrillocruz, haven't tested that recently | 17:49 |
| yolanda | sorry | 17:49 |
| *** MaxV has quit IRC | 17:51 | |
| mrmartin | yolanda, a quick question maybe you know, what is the proper way of alembic usage, if I want to create a new revision? | 17:59 |
| yolanda | hi mrmartin | 18:01 |
| yolanda | so we have the storyboard/db/migrations folder | 18:01 |
| mrmartin | yeap | 18:02 |
| yolanda | there is that directory inside storyboard/db/migration/alembic_migrations/versions/ | 18:02 |
| yolanda | so you create a new entry there, checking the oldest one | 18:02 |
| yolanda | i think that with aripinen changes we are on 035 | 18:02 |
| yolanda | then you just create a upgrade method and a downgrade method, something like that | 18:03 |
| yolanda | https://review.openstack.org/#/c/150447/6/storyboard/db/migration/alembic_migrations/versions/035_branch_id_in_tasks.py | 18:03 |
| krotscheck | rcarrillocruz: I haven’t looked at that in a while, I usually run the subscriber directly in a debugger. | 18:03 |
| mrmartin | so it means, that I need to write this source code manually? | 18:04 |
| krotscheck | mrmartin: Yep. | 18:04 |
| *** ssam2 has quit IRC | 18:07 | |
| nibalizer | hey everybody we're using storyboard for an infra sprint right now | 18:11 |
| yolanda | krotscheck, we' ve been talking with NikitaKonovalov about the validation problem, what's your opinion here? | 18:11 |
| krotscheck | I’m composing right now | 18:11 |
| nibalizer | and im really impressed at how easy it is to do mappings of people to subtasks | 18:11 |
| krotscheck | In the review. | 18:11 |
| nibalizer | +1 | 18:11 |
| krotscheck | nibalizer: It’s tabbing all the way down :) | 18:13 |
| yolanda | nibalizer, storyboard rocks :) | 18:13 |
| nibalizer | rocks my socks! | 18:13 |
| nibalizer | oh wait i can tab on this thing? | 18:13 |
| * nibalizer has been point/click like a pleb | 18:13 | |
| *** jedimike has quit IRC | 18:14 | |
| *** openstackgerrit has quit IRC | 18:14 | |
| *** openstackgerrit has joined #storyboard | 18:14 | |
| krotscheck | nibalizer: Yep, though there’s some questions on whether that’s going to stick around in its current form. | 18:14 |
| yolanda | i also love the tab feature | 18:15 |
| krotscheck | yolanda: Long story short- why are we trying to fix something that ain’t broke with more complicated code? | 18:15 |
| yolanda | and you talk about? validations? | 18:15 |
| *** SotK has quit IRC | 18:16 | |
| krotscheck | yolanda: Yep. | 18:16 |
| yolanda | so i think it's healthy to do validations, to prevent malicious code | 18:16 |
| yolanda | if someone is trying to manipulate fields, update something not allowed, etc | 18:16 |
| krotscheck | yolanda: To explicitly filter out fields for various requests is silly | 18:16 |
| *** jcoufal_ has quit IRC | 18:17 | |
| krotscheck | And a lot of effort. | 18:17 |
| yolanda | krotscheck, so you would just skip validation of fields? | 18:17 |
| krotscheck | That’s not what I’m saying. | 18:18 |
| yolanda | we could think that in the reverse way then... if we are interested that some fields cannot be updated for certain roles, have a validation rule for that | 18:18 |
| krotscheck | Not allowing created_at on a PUT or POST request just because it’s technically not a thing we don’t allow users to update is what I think is silly | 18:18 |
| NikitaKonovalov | ok, so looks like the soluion is that db-managed fields are passed in a PUT or POST the api should ignore them silently | 18:18 |
| NikitaKonovalov | if something like creator_id or even id is send and it does not match existing -> 400 error | 18:19 |
| krotscheck | I agree that using the json schema to enforce a certain set of fields and types is really useful. | 18:19 |
| krotscheck | Right. | 18:19 |
| krotscheck | With the exception that some fields are server owned. | 18:19 |
| krotscheck | UNLESS we want to go 100% RESTful | 18:20 |
| krotscheck | In which case there’s a lot more work we need to do :) | 18:20 |
| NikitaKonovalov | I guess we are not following standards that closely :) | 18:20 |
| yolanda | date_created, updated, are server owned, we don't have to update them if they are passed by the client | 18:20 |
| krotscheck | NikitaKonovalov: We’re really not :) | 18:20 |
| krotscheck | I’m not opposed to being 100% RESTful mind you. | 18:23 |
| krotscheck | But if that’s what we’re going to do, we need to get everyone on board. | 18:23 |
| NikitaKonovalov | so I guess the plan is that JsonSchema will continue validating the fields and enforce that request has what is should have | 18:23 |
| krotscheck | For now, yes. | 18:24 |
| NikitaKonovalov | and one more layer should check if the server-managed fields are passed in the request they should be checked | 18:24 |
| NikitaKonovalov | to match the existing in the model | 18:24 |
| yolanda | NikitaKonovalov, krotscheck, either that or just ignore the ones that are server managed? | 18:25 |
| yolanda | do we want to waste resources in validating that the server managed fields matches from backend and frontend? | 18:26 |
| NikitaKonovalov | I need to check what happens when the wrong id is send in the body | 18:26 |
| krotscheck | yolanda: Is not wasting resources a premature optimization? | 18:26 |
| NikitaKonovalov | if sqlalchemy tries to change that, that's a problem | 18:27 |
| krotscheck | NikitaKonovalov: I added a test for that, it appears that our API overrides that. | 18:27 |
| *** Piet has joined #storyboard | 18:27 | |
| krotscheck | When we do entity_update(id, stuff), it seems to handle that propelry | 18:27 |
| yolanda | krotscheck, it means an extra database access to check if date_created, date_updated, are sent different by the frontend, and why, if we are just going to ignore them? | 18:27 |
| krotscheck | yolanda: If we were handling our database sessions on a per-request basis rather than a per-query basis, that wouldn’t be a problem. | 18:28 |
| NikitaKonovalov | yep I've actually found that line https://github.com/openstack-infra/storyboard/blame/master/storyboard/db/api/base.py#L284 | 18:29 |
| krotscheck | Oh neat | 18:29 |
| NikitaKonovalov | so id will always be correct | 18:30 |
| NikitaKonovalov | that however does not apply to author_id of a comment or creator_id of a story | 18:30 |
| krotscheck | Is that a DB hook? | 18:31 |
| yolanda | so you both are in favour of double checking all server owned fields rather than just ignoring them? what's the benefit? | 18:31 |
| NikitaKonovalov | krotscheck: that's entity_update | 18:31 |
| NikitaKonovalov | yolanda: ignoring means there should be a declaration of what should be ignored for each model | 18:33 |
| NikitaKonovalov | and the reques should be checked against that every time | 18:33 |
| yolanda | cannot we have a general "server owned" rule? | 18:33 |
| yolanda | btw, you will need to have that declaration if you want to check if they are different, and raise a 400 ... | 18:34 |
| NikitaKonovalov | yolanda: there is a similar rule of public_fileds, which was actually added for User model to keep things like email from public access | 18:40 |
| NikitaKonovalov | we could have server_managed_fields for models that need them | 18:40 |
| NikitaKonovalov | and check requests with a hook | 18:41 |
| yolanda | well, thinking on your point of view, i can see the advantage that if we check for date_created, date_updated and they are different, it means that someone is trying to attack | 18:41 |
| yolanda | as it's manipulating the json | 18:41 |
| NikitaKonovalov | or trying to steel a coment by changing its author | 18:42 |
| yolanda | in that case you raise a validation error and don't allow the update of anything, i can see an advantge there :) | 18:43 |
| NikitaKonovalov | ignoring is simple solution here. there is no need to fetch an object for comparison | 18:43 |
| yolanda | ok so i'm fine with your proposal | 18:44 |
| krotscheck | I’d say ignore them. if we check for date_updated and it doesn’t match, it could just be that someone else modified the resource in a different API request. | 18:52 |
| NikitaKonovalov | krotscheck: good point | 18:53 |
| NikitaKonovalov | I guess we'll agree to ignore the fields which should be managed by server only. And all these fields should be declared in a model class (or may be wsme model class) and cleaned out from request with a hook | 18:54 |
| krotscheck | Or, in truly restful fasion, we could throw a 409 conflict and make the client handle the diff. | 18:54 |
| *** MaxV has joined #storyboard | 18:57 | |
| yolanda | krotscheck, -1 for that :) | 19:00 |
| yolanda | either ignoring or raise validation error has pros and cons | 19:01 |
| yolanda | if we ignore we may be missing attacks, if we raise validation error we may be invalidating proper requests as you point | 19:01 |
| *** MaxV has quit IRC | 19:01 | |
| *** peristeri has joined #storyboard | 19:13 | |
| peristeri | what is the tox arguments needed to run the test locally? | 19:16 |
| *** yolanda has quit IRC | 19:17 | |
| *** yolanda has joined #storyboard | 19:17 | |
| *** yolanda has quit IRC | 19:18 | |
| *** yolanda has joined #storyboard | 19:22 | |
| *** yolanda has quit IRC | 19:22 | |
| *** yolanda has joined #storyboard | 19:23 | |
| *** yolanda has quit IRC | 19:24 | |
| *** yolanda has joined #storyboard | 19:26 | |
| *** yolanda has quit IRC | 19:27 | |
| *** yolanda has joined #storyboard | 19:28 | |
| *** yolanda has quit IRC | 19:29 | |
| openstackgerrit | Merged openstack-infra/storyboard-webclient: Parse event_info into JSON. https://review.openstack.org/149126 | 20:10 |
| openstackgerrit | Michael Krotscheck proposed openstack-infra/storyboard: subscription_helper now supports timeline_events https://review.openstack.org/150170 | 20:22 |
| openstackgerrit | Michael Krotscheck proposed openstack-infra/storyboard: Singularized resource names. https://review.openstack.org/150169 | 20:22 |
| krotscheck | zaro: You had a comment here, without much of a suggested solution. Could you go into details on what you meant? https://review.openstack.org/#/c/147734/2/storyboard/plugin/email/factory.py | 20:34 |
| *** alexismonville1 has quit IRC | 20:39 | |
| *** Piet has quit IRC | 20:49 | |
| *** petefotheringham has joined #storyboard | 20:53 | |
| *** SotK has joined #storyboard | 21:18 | |
| *** SotK has quit IRC | 21:20 | |
| *** SotK has joined #storyboard | 21:26 | |
| *** mrmartin has quit IRC | 21:30 | |
| *** Piet has joined #storyboard | 21:31 | |
| *** yolanda has joined #storyboard | 21:44 | |
| *** mattfarina has quit IRC | 22:03 | |
| *** peristeri has quit IRC | 22:36 | |
| *** openstackgerrit has quit IRC | 23:06 | |
| *** openstackgerrit has joined #storyboard | 23:06 | |
| zaro | krotscheck: replied in the review. | 23:21 |
| krotscheck | zaro: Gotcha. | 23:31 |
| krotscheck | zaro: That’s what I figured. | 23:31 |
| krotscheck | In other news. SEriously, I can monkeypatch python? | 23:31 |
| krotscheck | Something about that seems wrong. | 23:32 |
| zaro | doesn't that apply to any oop language? | 23:33 |
| krotscheck | zaro: You can’t monkeypatch java at runtime I think. | 23:34 |
| krotscheck | zaro: As in, I can’t go in and say java.lang.Integer = MyOwnImplementation | 23:34 |
| krotscheck | (I think) | 23:34 |
| * krotscheck may be wrong. | 23:34 | |
| zaro | ohh maybe not the basic types, so not int, but i'm pretty sure you can for Integer | 23:36 |
| *** Piet has quit IRC | 23:38 | |
Generated by irclog2html.py 2.14.0 by Marius Gedminas - find it at mg.pov.lt!