Put the Python in the repo

[JWPlatt]

I was about to ask for a post-mortem, but I see MOULSCRIPT-7 is a restart. Any comments anyway?

[a'moaca']

I was about to ask for a post-mortem, but I see MOULSCRIPT-7
is a restart. Any comments anyway?

I am still not appreciating Crucible. The review part itself is okay, but...

I asked some questions in one of the reivews. By default it sends a separate email message for every little comment. I don't even want to know what it did when I edited one of them twice. Did poor D'Lanor get three messages, including the first two I didn't want? D'L said he tried enabling "Batch" mode, so I made some more comments to try it out, but I don't know if it worked for him.

When he tried to combine a later checkin into a review, I wasn't prepared for it in two ways: I just wasn't prepared to review all the security fixes (I thought I was doing something else), and second, I was concerned about attribution.

Rarified mentioned it here, but then... nothing. If we think proper attribution is important, how are we going to ensure it happens? I applaud D'Lanor's quick efforts to get these changes into our tree, but we've gone and stalled him by not having a good way to do it the way we want. How can D'Lanor do it better, or are we stuck with him owning the changes just because he imported them by hand?

Anyway, then D'L tried to add the files he'd missed out to the now combined review and the darned thing put *those* in another two review requests! That's why there's a restart, I'd guess -- I have not yet looked at the new one.

Furthermore, I'm a bit frustrated personally. I made time to review these because I thought as a temporary gatekeeper it was my job to review them and in a timely fashion so that the changes could be put in the main branch or tip or whatever you call it. With write access, I thought it was my job to propagate the changes. But then we had a brand new proposal for a non-existent process here, where the change's original owner would do it. So then I was stalled too, even on the changes with no attribution concerns -- suddenly I don't even know what MY job is.

How anyone else will figure this all out is beyond me right now.

- a'moaca'

[rarified]

Sorry A'moaca', I'm not trying to frustrate you. I've been intermittant because of work demands, and havn't had time to flesh out something coherent. I was hoping we could collaboratively explore the Crucible workflow (which I'm not familiar with either).

I'm at work so all I can say now is I'll explore the process more tonight and see if I can come up with the Readers Digest version. And we are using this experience to learn; I'm thinking things will be simpler if we stick to a "branch" model with only one repository, where proposed changes go into an open branch. Then the "merge" gets much simpler. Look for that change during the next set of changes to go in.

I'm still looking at a way to grant commit permissions to the originator of a change to the main branch automatically when Crucible notes that the review is completed, but that's not there yet. Until then we'll have to rely on the authoritative reviewers for help merging.

_R

[D'Lanor]

I pushed a few wrong buttons in Fisheye and reviews got tangled. There was no way to undo it other than restart. So the Recents fix is now open for review again.

The GoW security fixes apparently need no reviews because they already made it to MOULa. I suggest you put everything into the repo exactly as it was implemented by Chogon. That would include the name fixes from Paradox which he merged back in.

I would like to point out that me pulling in something from the GoW fork was an exception. The security fixes seemed pretty urgent so I quickly added them.
In the future I will be limiting my contributions to my own stuff. However, don't expect much from me until I have a proper testing environment to play around with. Even though my fixes are known to work in Alcugs and UU they are at this point not tested in a MOULa environment. I mean, code reviews are useful but I had hoped someone here with the proper tools would be able to do the actual testing. Preferably before this gets committed to the main branch.

[Mac_Fife]

Testing is something that had been on my mind - e.g. what would we consider to be adequate testing?

I haven't had time to look at your changes at all D'Lanor, so apologies if the answer to this is self-evident: If someone with the relevant tools were to pick them up, is it fairly obvious from the changesets what they should be looking for in performing any tests? Or will you provide notes on request?

[D'Lanor]

I think it should be obvious but as the author of the changesets I may be biased. You would need multiple players though, since these are multiplayer bugs.

[JWPlatt]

Just a note here on something a'moaca' and rarified mentioned: The idea of a single repo is that it will allow pushes to developer branches by any qualified developer to a non-default branch. The developer branch would later be merged into the default (stable or main) branch after a changeset has gone through review and testing. There are still "gatekeepers" (select persons with commit rights) on the default branch. What kind of options there are to streamline things, if at al possible, for the gatekeeper with a commit proxy to the submitter for the specific changset is something rarified is looking into. It does not open the default branch to any developer. It is meant to make it easier on the gatekeeper, but retain integrity within the process.

We can already do the single repo with gatekeeper access to the default branch and developer access to any other branch. We want to see how the review process works on the separate "-dev" repo first before refining things. One test at a time for better isolation of trouble.

[Nye_Sigismund]

The GoW security fixes apparently need no reviews because they already made it to MOULa. I suggest you put everything into the repo exactly as it was implemented by Chogon. That would include the name fixes from Paradox which he merged back in.

I support this. Code that gets on to MOULa should be considered already reviewed. It's tested properly.

I think that going forward, the tools that OU are using need to be properly fleshed-out and assessed. If they're simply too complicated, too easy to break or whatever, then they'll be unsuitable for the distributed development model that OU wishes to have in place, even if they're powerful. I've personally had no problem with the repo and JIRA, but if code review is problematic then OU needs to sort it out.

[JWPlatt]

I agree that if it's running on MOULa and was tested by Cyan, it should be on the default (main, stable, whatever) branch/trunk, and can get committed right away. The repo should reflect what's on the MOULa shard. This circumstance was special because the change was a security fix. Mark delivered the revisions to us and I just haven't pushed them yet, so D'Lanor actually beat me to it. I was busy with non-OU things over the weekend. I still am, but intend to give some love to OU as quickly as possible.

[a'moaca']

I'm still looking at a way to grant commit permissions to the originator of a change to the main branch automatically when Crucible notes that the review is completed, but that's not there yet. Until then we'll have to rely on the authoritative reviewers for help merging.

So I think you are saying I should propagate D'Lanor's changes? I mean, aside from him wanting testing first? Also, if you make this automatic, then the code author can't be the moderator, since the moderator is who closes the review.

I mean, code reviews are useful but I had hoped someone here with the proper tools would be able to do the actual testing. Preferably before this gets committed to the main branch.

I find it curious that there are still no public MOSS servers set up. (I wonder if the state of the client is part of it?) Anyway, is OpenUru.org supposed to be in the testing business? If so, we need our own server. Support staff to get changes onto the server. Volunteers to get together and do multiplayer testing. I guess I thought that there would at least be some interest in this in the greater community; there was enough of it to go around in UU.

I agree with everyone about the security fixes. Past experience tells me Chogon reviewed them. This is the other part of why I was not prepared to review them -- they have been reviewed. I even considered propagating them, but for my question about the attribution. I'm now unsure whether we are worried about that, but if JW has the version Chogon implemented, I'll leave it for him to sort out!

Which brings me to another thing we need from the foundry. Currently the MOSS setup instructions point people to the compiled Python we received from Cyan earlier. Now we should be producing a version of our own. Mystler has even done the work of getting the tools working!

- a'moaca'