15:00:41 <mfedosin> #startmeeting glare
15:00:42 <openstack> Meeting started Tue May 16 15:00:41 2017 UTC and is due to finish in 60 minutes.  The chair is mfedosin. Information about MeetBot at http://wiki.debian.org/MeetBot.
15:00:43 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
15:00:46 <openstack> The meeting name has been set to 'glare'
15:01:24 <mfedosin> inarotzk: hi :)
15:01:40 <inarotzk> Hi :)
15:01:41 <mfedosin> #topic agenda
15:01:55 <mfedosin> #link https://etherpad.openstack.org/p/glare-meeting-agenda
15:02:34 <mfedosin> #topic Updates
15:03:31 <mfedosin> so, let's speak about our current updates...
15:03:45 <mfedosin> Frankly speaking I don't have many
15:04:09 <mfedosin> I finished the first part of documantation about glare artifact type framework
15:04:24 <mfedosin> #link https://review.openstack.org/#/c/465075/
15:05:14 <inarotzk> awesome , I'll review it
15:05:15 <mfedosin> also I reviewed inarotzk code a little
15:05:44 <mfedosin> finally I have a big patch with unit tests for uploading
15:05:57 <mfedosin> but I haven't sent it on review yet
15:06:11 <mfedosin> I think that's all from my side
15:06:37 <mfedosin> inarotzk: I saw you proposed a lot of patches
15:07:13 <mfedosin> we can discuss them if you want
15:07:31 <inarotzk> Sure
15:08:01 <inarotzk> my last updated patches are: https://review.openstack.org/#/c/463767/4 - Add Validator to visibility field,
15:08:06 <inarotzk> https://review.openstack.org/#/c/464320/4 - Merge 2 lines into 1 line
15:08:13 <inarotzk> https://review.openstack.org/#/c/465076/  - add InvalidException class
15:08:17 <inarotzk> https://review.openstack.org/#/c/464978/1 -Remove unnecessary spaces
15:08:57 <mfedosin> oh yeah, thank you for the links :)
15:09:18 <mfedosin> #link https://review.openstack.org/#/c/464978/1
15:09:35 <mfedosin> ^ I think this one is good.
15:09:47 <mfedosin> and we should merge it
15:10:14 <inarotzk> i did "Abandomsome" to some of my unwanted patches. Is it fine to do so?
15:10:19 <mfedosin> just one thing, that is not related to the patch itself
15:10:26 <inarotzk> yes?
15:11:00 <mfedosin> it not recommended to put +2 on your patches
15:11:16 <inarotzk> oh, okay :)
15:11:29 <mfedosin> if you want to signalize that you think your code is good then put +1 instead of +2
15:11:59 <mfedosin> It's just a polite practice.
15:12:10 <inarotzk> Yes, i'll do it from now on
15:12:39 <inarotzk> *** "Abandom" to
15:12:45 <mfedosin> about abandon patches - yes, if you feel that this patch is not required, you abandon it
15:13:47 <mfedosin> https://review.openstack.org/#/c/464978/1 <- I +2A'ed this patch
15:14:10 <inarotzk> I'll be glad to hear what you think about this patch - https://review.openstack.org/#/c/465076/ -InvalidVersion exception
15:14:55 <mfedosin> yes, I see an issue there
15:15:44 <mfedosin> As you can notice all exception classes just add 'message' and use __init__ from parent class
15:16:23 <mfedosin> and I think you should do the same
15:17:14 <mfedosin> I see that in the code it uses 'reason' parameter
15:17:18 <mfedosin> not 'message'
15:17:19 <mfedosin> https://github.com/openstack/glare/blob/b07bc1a78b56d28f99ed41202c65c118c78412db/glare/common/semver_db.py#L126
15:17:56 <mfedosin> and my suggestion to change it in 'semver_db' module
15:18:01 <inarotzk> I get the message in initialization part. So i guess i can remove it from the common/semver_db.py class, and use the same message
15:18:20 <mfedosin> i.e. write it like raise exception.InvalidVersion(message=reason)
15:19:08 <mfedosin> and the same here https://github.com/openstack/glare/blob/b07bc1a78b56d28f99ed41202c65c118c78412db/glare/common/semver_db.py#L78
15:19:52 <inarotzk> Than, still we have different format in exception. Don't we?
15:19:52 <mfedosin> also it's recommended to add some common Exception message for cases when 'message' parameter hasn't been provided at all
15:19:58 <inarotzk> *then
15:20:25 <inarotzk> Ohh, i see
15:20:57 <mfedosin> no, format will be the same
15:21:27 <mfedosin> I'll a comment after this meeting
15:21:30 <inarotzk> okay, I understood
15:21:39 <mfedosin> don't need to change it right now :)
15:22:35 <mfedosin> yes, the idea is to create an exception class InvalidVersion with some abstract message
15:22:38 <inarotzk> another question, Is regarding the exception itself ,which is risen when i do sorting test (only in py35)
15:23:01 <mfedosin> and change parameter name from 'reason' to 'message' in semver_db module
15:23:18 <mfedosin> inarotzk: frankly speaking no idea
15:23:28 <mfedosin> I haven't tried it yet
15:23:39 <mfedosin> it will be an action for me
15:24:00 <inarotzk> The error is: version component is too large (65535 max)
15:24:18 <mfedosin> #action (mfedosin) check why sorting tests fail on py35
15:24:18 <inarotzk> NP
15:24:57 <mfedosin> I think you have another patch not related to unit tests
15:25:52 <mfedosin> https://review.openstack.org/#/c/463767/4
15:26:14 <mfedosin> but I don't see it's changed from my last review...
15:27:06 <mfedosin> you said "Done" and rebased the patch two times
15:28:14 <inarotzk> Yes. I think that is happended because i  rebased a branch with related commits
15:28:25 <mfedosin> also I see that there is a patch that adds changes required by me
15:28:50 <mfedosin> https://review.openstack.org/#/c/464320/4
15:29:11 <mfedosin> probably we can squash them together and resent again
15:29:24 <mfedosin> if you need help on this matter I can help
15:30:07 <mfedosin> and as I mentioned there code looks good and we can merge it
15:30:34 <inarotzk> You mean squash :  Merge 2 lines into 1 line & Add Validator to visibility field & Delete unnecessary spaces ?
15:31:48 <mfedosin> yep
15:31:57 <mfedosin> exactly :)
15:32:16 <mfedosin> okay, we can discuss it later
15:32:18 <inarotzk> I did abandon to "Delete unnecessary spaces" since it related to other change (sorting test list), and i insert this change over there.
15:33:02 <inarotzk> And regarding "Merge 2 lines into 1 line & Add Validator to visibility field" , Do you think it's better in one commit instead of two?
15:33:37 <mfedosin> yes, sure
15:33:48 <inarotzk> OK. NP
15:33:51 <mfedosin> why do we need two separate commits?
15:34:03 <mfedosin> merge two patches in this one https://review.openstack.org/#/c/463767/4
15:34:20 <mfedosin> and we'll add you code to master
15:34:25 <mfedosin> inarotzk: thanks :)
15:34:46 <mfedosin> okay, let's go to tests
15:34:57 <mfedosin> #topic Tests and coverage
15:35:20 <mfedosin> there are several my patches on review
15:36:01 <mfedosin> 1 https://review.openstack.org/#/c/463321/1
15:36:27 <mfedosin> this one changes default tox cover interpreter to py27
15:36:41 <mfedosin> because cover doesn't work with 3.5
15:37:02 <mfedosin> and also it removes test modules from calculations
15:37:15 <mfedosin> i.e. it leaves only code related things
15:37:30 <mfedosin> about this one: https://review.openstack.org/#/c/463380/1
15:37:40 <mfedosin> there are wsgi tests
15:38:04 <mfedosin> I think we found out the reason of inarotzk questions
15:38:33 <mfedosin> the was a bug with has_body method
15:38:47 <mfedosin> which returned None instead of False
15:38:56 <mfedosin> inarotzk fixed it yesterday
15:39:37 <mfedosin> https://review.openstack.org/#/c/464655/
15:40:06 <mfedosin> inarotzk I hope now we can merge the whole chain of commits:
15:40:14 <mfedosin> https://review.openstack.org/#/c/463321/1
15:40:21 <mfedosin> https://review.openstack.org/#/c/463380/1
15:40:28 <mfedosin> https://review.openstack.org/#/c/464655/1
15:42:17 <inarotzk> yes, i think we can :)
15:42:18 <mfedosin> Hooray! Thank you!
15:42:46 <mfedosin> Also I reviewed you tests for sorting and it's good
15:43:44 <mfedosin> but as said before it will be good to add some negative scenarios too
15:44:20 <inarotzk> yes, I am currently working on it
15:44:46 <mfedosin> great
15:45:31 <mfedosin> and also I would recommend not to create new changes, but add new patches to the existing code
15:45:49 <mfedosin> it's much easier to review
15:46:14 <mfedosin> because now I see 3 commits for sorting tests
15:46:32 <mfedosin> one of them is abandoned and two on review
15:46:56 <mfedosin> okay, np. we'll fix that
15:47:01 <inarotzk> In order to to do so, i should git review -d {branch_number}. Am i right?
15:47:30 <inarotzk> And then we will have new patch. Is that what you mean?
15:47:58 <mfedosin> yes: git review -d {branch_number}, make you changes, git commit -a --amend, git review
15:48:21 <mfedosin> it will create new patch
15:48:52 <mfedosin> for example: https://review.openstack.org/#/c/463380/1
15:49:26 <mfedosin> "1" in the end is a patch number
15:49:57 <mfedosin> when you add another change there will be "2" and so on
15:50:07 <mfedosin> this one has 4 patches https://review.openstack.org/#/c/463767/4
15:50:32 <mfedosin> okay, we don't have too much time left
15:50:35 <inarotzk> okay, cool
15:50:50 <mfedosin> I'll make a quick update about the documentation
15:50:58 <mfedosin> #topic Documentation
15:51:32 <mfedosin> I made a small doc that describes the basics of glare artifact type framework
15:51:39 <mfedosin> it doesn't cover too much
15:52:02 <mfedosin> but it says how to create new artifact types and fields for them
15:52:26 <mfedosin> in next one I'll write about field parameters
15:52:45 <mfedosin> 'system' 'required_on_activate' and so on
15:52:57 <mfedosin> okay, main topic
15:53:03 <mfedosin> #topic Big Tent
15:53:23 <mfedosin> inarotzk: what do you think?
15:53:34 <mfedosin> should we apply?
15:53:39 <mfedosin> and when?
15:54:01 <mfedosin> technically we can do it this or next week
15:55:08 <inarotzk> yes, i think we should talk in the next few days and decide
15:55:47 <mfedosin> agreed :)
15:56:05 <mfedosin> #topic Open Discussion
15:56:24 <mfedosin> do we have any uncovered topics left?
15:57:02 <inarotzk> mm, I don't think so
15:57:44 <mfedosin> okay, so we can finish for today
15:57:51 <mfedosin> thank you for joining!
15:57:57 <mfedosin> and have a good week
15:58:12 <mfedosin> #endmeeting