HEADS UP: Changes to the PULL process

HEADS UP: Changes to the PULL process

by Martin Dougiamas -
Number of replies: 20
Picture of Core developers Picture of Documentation writers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Plugin developers Picture of Testers

Please note that our PULL process for reviewing code for integration has changed.

The workflow for pulls has now been integrated into the main workflow for MDL issues, and is no longer a separate PULL project in the tracker.

The main benefits of this are keeping it simpler for users, and reducing the problems of discussions being spread across multiple places.  Viewing the History tab will give you a complete history of the issue, including failed PULLs.

For more details about the new workflow see http://docs.moodle.org/en/Development:Process in Moodle Docs.  It's still being tweaked/improved too so please let me know of you find any problems.

Average of ratings: -
In reply to Martin Dougiamas

Re: HEADS UP: Changes to the PULL process

by Anne Krijger -

Hi Martin,

Although it may not be directly related to the PULL process, the actual development cycle of Moodle is not quite clear to me yet.

If I understand correctly there is a set of developers who have the right to change the core code of Moodle. This set is called the 'Developers' in the documentation you referenced.

What I don't quite understand is how a developer like me, who is not part of that group, can contribute core code changes.

The way I understood it was that
an issue is made,
the code change attached to it as a patch
and to make people aware of it a forum message created inviting people to vote for the issue.

That's what I did, but apparently nobody is interested in my fix smile
To be honest, I don't think I would be if I hadn't run into this (small) issue myself.

The problems I see though are;

1) Because the patch was made against the most recent GIT version available at the time, and the change touched quite a bit of code in a specific file, the patch may not be usuable anymore.

2) Fixes that do in fact fix code but are of no interest to most people may not be included in a next release.
This means that for every coming release I have to check if my patch is included and if not hope I can still use my patch myself.

So, is there a way around these two possible pitfalls ?

Anne.

Average of ratings: -
In reply to Anne Krijger

Re: HEADS UP: Changes to the PULL process

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers

1. Acutally, patches continue to apply for a surprisingly long type, normally.

An even better alternatively to making a patch is to instead make a branch on a public git repository, where people can pull your changes from.

 

2. Getting all fixes reviewed and included is a problem. Hopefully it is gradually getting better. Again, using git to publish, and then later merge the changes works a little better than using raw patches.

Average of ratings: -
In reply to Anne Krijger

Re: HEADS UP: Changes to the PULL process

by Martin Dougiamas -
Picture of Core developers Picture of Documentation writers Picture of Moodle HQ Picture of Particularly helpful Moodlers Picture of Plugin developers Picture of Testers

Hi Anne,

I am guessing you are referring to MDL-27212.  You've done everything right there, thank you.

Someone in triage should see it soon and check that the fields are right.  In this case it looks more like a STABLE issue than a DEV issue to me.

Urgent STABLE bugs would get picked up by the STABLE scrum team eventually, looked at and submitted to the integration process for you.  If you want to retain commit credits you should provide patches as links to your git repository (there are fields for this in the issue, now), otherwise the STABLE developer checking it in will get credit in the repository history (they should at least mention your contribution in the comments however).

Minor STABLE bugs will usually get picked up by the component lead (in this case whoever is reponsible for Tags) and they will review it and give you feedback.  If it is OK then they may pass it on for integration, otherwise the patch may need more work.

After a quick look I can see there are problems with variable names using camelCase, which do not follow the Moodle Coding Guide.  Obviously a proper review would look a lot deeper.  These are the kinds of things we need to prevent getting into Moodle core to build a high level of consistency and quality.

It might be some time before your particular bug can be processed, though, simply because of a lack of people in the reviewing and component lead roles.  That's obviously not your problem, it's ours (mine), and is something I am actively working on by hiring new people at Moodle HQ and seeking out volunteers to be component leads.

First we had to get the process worked out, though, and I'm confident that it's a good thing for Moodle.

Average of ratings: -
In reply to Martin Dougiamas

Re: HEADS UP: Changes to the PULL process

by Anne Krijger -

Hi Martin,

Yes I've used MDL-27212 as a test case to see how the process works.
If things go as planned, I expect I may be doing some more work on Moodle which may also include some core code changes.

BTW, feel free to point me elsewhere. I more or less 'hyjacked' this thread as it was the 1st I saw about the development process.

While creating the issue it was unclear to me which version(s) to choose for Affects as well as Fix.

I did a GIT clone of Moodle at that time and created a patch using diff against that version. But as said; I didn't find a reference anywhere as to what version that particular GIT clone was so I just guestimated what the versions were/should be.

I dn't know what the difference is between STABLE and DEV, but I guess that's for the triager to decide smile

I'm still faily new to GIT and it's workings so I would not know how to provide a patch as link to  my repo. Then again I really don't mind someone else submitting this under their name. I'll ask the GIT guru's here if they can explain the GIT process a bit more.

AFA comments on the issue go, can I recognize somewhere that the person commenting is the component lead? I obviously take every comment seriously, but might need to take the comments of the component leader even more seriously.

Funny you should mention the cameCasing. I did ask some php Guru's about this recently and they mentioned that while it's not often used, they themselves do and there was no reason not to.
I see it's mentioned in the Coding Guideline so I better not either.

So now we've established that the patch needs at least some work.
What is the flow from there?

Do I create a new patch without the camelCasing and attach it instead of the one currently attached?
Or do I add one?
Or do I wait untill a thorough review has taken place and then fix all the issues and provide a new patch?

I understand that resources are limited and that, especially minor fixes like MDL-27212 it will take some time before ending up in the release.

As I now understand it any issue will be triaged at a certain point in time. Because of limitations of resources that may take some time.  That's life smile

Anne.

Average of ratings: -
In reply to Martin Dougiamas

Re: HEADS UP: Changes to the PULL process

by Myles Carrick -
hi Martin,

A week or so when posting bugs I was delighted to see that I could submit a URL for a Git diff... but today that's gone. Did I perhaps just go there while you were testing/configuring? It was really great to be able to do that (especially for cases like this where I've fixed something locally prior to making the bug report.

Cheers - loving the new, timed releases and development workflow!

Myles C.
Average of ratings: -
In reply to Myles Carrick

Re: HEADS UP: Changes to the PULL process

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers

Myles. The workflow still seems to be being tweaked and refined (and there were definite bugs with it this week.)

The way it works now, you cannot add those URLs when creating the issue, but you can when you change the state to 'Ready for peer review'. You can also add the URLs at any time if you have permission to edit the issue.

Finally, you can always put any URL in a comment.

Average of ratings: -
In reply to Martin Dougiamas

Re: HEADS UP: Changes to the PULL process

by Derek Chirnside -

Martin, this is not a problem with the process as such, it is just a question.

You have bugs and issues and major new features.

What about smaller items that are not bugs, and are not part of the major projects that are beikng worked on?  Where do they fit in the process?

The case in point that led me to this post was Bulk course creation.  It does not seem to fit into the process.

-Derek

Average of ratings: -
In reply to Derek Chirnside

Re: HEADS UP: Changes to the PULL process

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers

Actually, if you look in the tracker, there types of issue are Bug, New feature, Improvement and Task.

Where did your list come from?

Average of ratings: -
In reply to Tim Hunt

Re: HEADS UP: Changes to the PULL process

by Derek Chirnside -

Tim, yes I know.  I was responding to Martin's post where he said: "For more details about the new workflow see http://docs.moodle.org/en/Development:Process in Moodle Docs.  It's still being tweaked/improved too so please let me know of you find any problems"  That's where the list came from, and doesn't feature 'improvements'.

I stumbled on the post when I was looking at an issue that was number 5 in the all time tracker voting and wondering why it was still around.  'Bulk course upload' for a middle sized moodle: too big to really do it (all the course creations) by hand and too small to have coders to script or implement authentication off a student database.  I thought Moodle could benefit by it being in the core.

I'm not even sure I undertstand the dev process fully, but as it was yesterday it does not take into account picking off a few 'easy wins' in the 'improvement' area every so often.  I have only just figured out PULL.  smile

I'm used to a tough, very very tough regieme.  Probably closest to MOSCOW.  http://en.wikipedia.org/wiki/MoSCoW_Method

Must, Should, Could, Won't

During working for 8 years on our home grown LMS, now unsupported - pet enhancements - Nope, won't.  or Done.  Or here in the priority.

A common word often used in the tracker/forums in dealing with improvements like this is 'hope'.  "We 'hope' to have that in version 2.x"  I'd rather someone say, 'Nope, not this year, but you are (at the moment) 7th in the queue".

What should happen to improvement tracker items doesn't feature in the process at the moment.

-Derek

Average of ratings: -
In reply to Derek Chirnside

Re: HEADS UP: Changes to the PULL process

by sam marshall -
Picture of Core developers Picture of Peer reviewers Picture of Plugin developers

The problem with queues is that if you are 7th in the queue for next year, by the time next year rolls around, you are likely to be at least 27th in the queue. The year after that? 77th. Etc.

IMO (I'm cynical) the expectation should be that in most cases, an improvement request will usually not be developed unless you (or somebody outside HQ) has the capacity to develop it (and to work with HQ when doing so to ensure that it meets their requirements).

Of course, some improvement requests do get developed by HQ.

--sam

Average of ratings: -
In reply to sam marshall

Re: HEADS UP: Changes to the PULL process

by Oleg Sychev -
Picture of Core developers Picture of Plugin developers

Hi, Sam.

You're not cynical enought. Did you really believe you capacity to develop it and work with HQ changes much? IMO not really, at least by the experience of last two years...

Average of ratings: -
In reply to sam marshall

Re: HEADS UP: Changes to the PULL process

by Derek Chirnside -

Sam, if this is so, then it needs to be in the process.

I'd rather know I was 77th than what happens currently.

What is sad is that some of the relatively minor issues are actually there with patches.

"Work with MoodleHQ" that's what all this process is about.

Average of ratings: -
In reply to Derek Chirnside

Re: HEADS UP: Changes to the PULL process

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers

You can help make patches more visible by:

  1. Add the label 'patch' to the issue.
  2. If you are the one who the patch belongs to, move the bug into the 'Waiting for peer review' workflow state, and if the relevant component has a component lead, you could add their name to the Peer reviewer field.
Average of ratings: Useful (1)
In reply to Tim Hunt

Re: HEADS UP: Changes to the PULL process

by Bob Puffer -

What would be the steps if you've offered up a patch for an issue which has been assigned to someone else but the issue is four years old and now the patch has been languishing in the tracker for over six months and nothing has taken place on it even though there are a lot of votes (27) and is marked "Major" and you've posted comments desiring somone to move it along but nothing happens so you don't feel much like working on a 2.x version of the fix even though you've had it in your own 1.9x production for over a year now?

Average of ratings: -
In reply to Bob Puffer

Re: HEADS UP: Changes to the PULL process

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers

Presumably you are talking about MDL-9085.

The trouble with an issue like that is that it is not clearly an improvement. If it was changed, it would make some people happy, and piss other people off.

If you want that change in standard Moodle, you need to come up with convincing evidence that the change is better for everyone. Otherwise you have to live with it as a customisation in your local Moodle.

The other thing you should do if you want a patch in standard Moodle si make sure you strictly follow the coding guidelines. On a quick glance at your patch, I see a lot of inline JavaScript, and comments that would not be appropriate were this to be applied to standard Moodle. It also leaves commented out code floating around.

I don't know enough about the assignment code to know if it is doing the right thing, but you do appear to have added even more levels of nested if statement to some already very complex logic.

Average of ratings: -
In reply to Tim Hunt

Re: HEADS UP: Changes to the PULL process

by Joseph Rézeau -
Picture of Core developers Picture of Particularly helpful Moodlers Picture of Plugin developers Picture of Testers Picture of Translators

Tim "The trouble with an issue like that is that it is not clearly an improvement. If it was changed, it would make some people happy, and piss other people off."

Well, that bug report has 27 votes for it. Considering that the vast majority of moodle users do not bother to vote in the bug tracker, that seems to me to be a significant number of users who do not like that "boring" dropdown list...

Too many clicks...

Joseph

Average of ratings: -
In reply to Joseph Rézeau

Re: HEADS UP: Changes to the PULL process

by sam marshall -
Picture of Core developers Picture of Peer reviewers Picture of Plugin developers

I haven't looked at this particular issue, maybe in that issue it is an obvious improvement, but in general that argument is surely a fallacy.

For example, 6.1 million people just voted 'Yes' in a UK referendum about changing to a slightly better voting system. 6.1 million is a lot more than 27, so clearly this should go through, right?

Which would have been be nice, but unfortunately 13.0 million people voted 'No'.

Same thing applies to Moodle bug tracker, but more so - nobody who is happy with the status quo would ever know about a bug to vote 'no' on it, even if there were a way to vote 'no', which there isn't.

So in other words bug tracker voting is only useful where an issue is a distinct improvement in that it would be unlikely to be opposed by, or cause problems for, any significant portion of users. If an issue doesn't meet that category (as Tim argued) then it doesn't matter whether there are 2 votes, 27 votes, or 6.1 million votes. smile

--sam

Average of ratings: -
In reply to Tim Hunt

Re: HEADS UP: Changes to the PULL process

by Bob Puffer -

Thanks, Tim for taking the time to check this out and provide your observations.  I will make the modifications you suggest.  I think what I was focusing most on was Martin D's comment:

Martin Dougiamas added a comment - 01/Sep/10 11:41 PM

Probably the reason this got left so long is that 2.0 included major plans to make ALL the grading interfaces in modules actually be defined by an API in the core gradebook (a little like how groups work). Unfortunately this got pushed to 2.1 because of the other million new features. But a quick fix that doesn't change too much might be ok to solve the major annoyance here.

And you're right about unlimitedgrades, my mistake.

 

I'm wondering, as an informed member of the core team, what you believe is a measurement of "convincing evidence that the change is a benefit to everyone"?  We have a long-standing issue (back to 1.3) that has no easy workaround (hacking a dropdown to 250 or 500 points in length), was important enough to suppose to have been dealt with in 2.0 and yet can't get any movement on it.  Martin D. was actually the one who escalated the issue from "Trivial" to "Major".  What should be my next step if I don't want to give up on it yet?

Average of ratings: -
In reply to Bob Puffer

Re: HEADS UP: Changes to the PULL process

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers

I am not really a member of the core team. I have just been around for a long time, and am component maintainer for the quiz and question bank.

What do I suggest you do?

  1. Well posting in this thread, and showing that you want to work constructively with the core developers even though they have ignored you for ages is a great way to start.
  2. You should make a new version of the patch that
    • scrupulously follows all the coding guidelines;
    • applies cleanly to the lastest master branch from git; and
    • if you can publish the change by making it available in a public git repository (https://github.com/timhunt/moodle/compare/MDL-26714_master...MDL-27155_master) then that makes it simplest for core Moodle developers to process. Otherwise, just attach an updated patch to the tracker.
  3. You could post in the assignment forum, explaining the change your are proposing to make (with screenshots, so people can see exactly wha the change is). If no-one in that forum objects within a week or two, you have a very strong case that the 27 yes votes in the tracker are not matched by an equal number of un-recorded no votes.

Once you have done all that, it will be quite hard for Moodle HQ to ignore you.

 

Average of ratings: -
In reply to Tim Hunt

Re: HEADS UP: Changes to the PULL process

by Hubert Chathi -

Unfortunately, it seems that regular users cannot move the bug into the "Waiting for peer review" workflow state.

Average of ratings: -