Made a few things current in the Sangoma wiki today to help new contributors take a walk through the GitHub procedures to submit patches step-by-step:
2 Likes
This method is not the correct way to handle this. Making a directory to download individual modules into and then firing up VSCode (or your favorite editor) then making some code changes, then making a commit, pushing it and creating a pull request is just not the right way.
This method does not allow for actual testing or initial QA by whoever is making changes to the module in question. This is a problem we’ve been trying to address in the community and this doesn’t help that problem.
Additionally, at the end of this document it says
The above steps go well together with Setting Up a FreePBX 17 Development Environment
This is not true. The development environment changes how the system fundamentally works. It expects the development to be done in an environment that allows for interacting with all the modules in the development environment, including having the other modules code that is in a development mode (such as other features/fixes/enhancements being worked on) allowing for testing, QA and validation that the code is working and no causing issues across the system or introducing any regressions.
The proper development environment doesn’t involve forking individual modules and working on them individually with no testing, QA or validation of these changes.
Oh in other news, the devtools and development setup for v17 doesn’t work properly at all. Following the documentation breaks the system and makes it unusable.
5 Likes
Maybe getting an actual developer to work on development docs and the broken development environment would be a good use of Sangoma resources versus your broken process you are trying to promote.
5 Likes
From the document in question:
- Fix the issue and test, test, test.
- Run
git diffto review your changes.- Run
git add .to prepare/group all of your changes.- Run
git committo type up a message describing your changes.
Will work on expounding on “test, test, test” a bit during the work week, thanks! That said, everyone has different preferences – your test environment might look like one or a combination of:
- devtools repo where patches with your ideas are welcome
- pngnx23299 installer for ansible (a non-official effort last year by YT with a minimal basic set of modules – which if you are working on one of those modules that is outside the basic list, then you could probably bundle it up and install via fwconsole – poking in on this project over the weekend (today) to look into updating it…)
- installing FreePBX with the normal shell installer for v17, then traversing to the module you want to poke around with,
git initin there, “test, test, test”, then take the final diff back in to your pristine GitHub cloned module repo before pushing back upstream and submitting your PR (might be fine for minor PHP/documentation updates that don’t touch the database schema, clobber other modules, etc.)
Sangoma does internal QA and validation, with multiple teams of people involved, before tagging public module releases that you see in the Module Admin GUI page. Often this is via CLI eg. fwconsole ma downloadinstall MODULENAME --tag TESTVERSION to test a specific module with a new specific version but depends on the needs of the different engineering teams involved (hmm 
future blog post (?)).
Another nice thing about the diversified development approach in such a robust project like FreePBX is that it helps uncover more bugs than a singular one-size-fits-all solution.
I just read a lot of words that say we have no test plan. Software engineering and specifically software testing has nothing to do with a bunch of random people saying the bit I need works. So there is that one feature none of your teams use so it doesn’t ever get tested. I think a perfect example of this is that one of the most critical modules backup has been a complete disaster in 17.
Software testing the tl;dr version.
- I have a module that does features A, B, C.
- In order for features X to work we must do Foo and bar
2a. Foo is a form. Are all fields of the right type. Does validation work. Does it meet uniformity standards. If code review does this meet coding standards. Can it be localized, does it have the right naming structure, do the fields in the frontend and backend match
2b our form submitted do the data structures look right, when you view the saved results so the items reflect what was submitted. If we edit and update does everything update. If we delete does everything delete. Was a reload required, did it set the reload button. - Go through and check all documented functions. Do they work as intended? Is there a change in expected functionality? Is that documented? Did we add functionality? Is that documented and has the test plan been updated to include that functionality with dependent actions.
- All features have been tested for this release and all code has been reviewed. Release it…
Software testing isn’t a make it up as you go and fly by the seat of your pants kinda thing. There should be a full uniform test plan that the developer and QA person follows. Basically the QA person should be a failsafe because the developer already tested things to a documented standard. The cool thing is once you have these standards in place a lot of it can be automated. You can put git hooks in place that will handle most coding standards and prevent a commit where standards aren’t followed.
In a perfect world you would do test driven development and write your tests before you write a single line of code. It is not a style of coding I have ever been able to wrap my brain around so I wouldn’t put anyone to that standard. That said testing should be a documented, consistent, reproducible thing that even your grandmother could do.
4 Likes
It also makes collaboration harder. Instead of the developers working from a single repo we have a bunch of individual repos that have to be forked and followed. The concept of the Development Environment was to avoid that. @jfinstrom and I could both work on a module together and others could join in. If I was working on something in my branch and I needed a second pair of eyes, @jfinstrom could just switch to that branch and provide those additional eyes. Additionally, if we find we are both working on something similar we can simple decided to merge my version into @jfinstrom’s or vice versa and all the other developers would have access to it.
We shouldn’t be working out of multiple repos and forks, it’s just an ingredient for confusion.
1 Like
IIRC multiple repos (one per module) have been a thing in FreePBX for a long time – although maybe that approach is due for an update.
As for forks, making one in your own account is what Asterisk recommends as well. Sharing your branch with others happens as soon as you push back your local dev changes to github, allowing them to run FreePBX Unit Tests, QA, etc., and soon your own git hooks like for example Asterisk does as well.
What kind of update?
This is edited, clarified, and expanded by ChatGPT to remain in compliance with the community code of conduct.
Imagine each module is like a Lego set. In the old way of doing things, all the Lego sets were kept in one big box. If someone wanted to work on a specific Lego set, they would just take it out of the box, work on it, and put it back in.
Now, with GitHub’s model, each Lego set has its own box. If you want to work on a Lego set, you need to take the box to your own room, work on it there, and then bring it back.
This creates two problems:
-
Switching tasks is harder. Before, you could just grab a different Lego set from the big box. Now you have to go to a different room to get the right Lego box.
-
You have to remember where the Lego boxes came from. Each box has its own “home,” and you need to remember where to bring it back when you’re done.
In tech terms:
Before: All changes were made in one place, like FreePBX/framework@FREEPBX-1234.
Now: Each person has to create their own version of that place, like jfinstrom/framework@FREEPBX-1234.
This means there are now two “upstreams” (original places the work came from), and you can’t just switch between tasks easily because they’re in different locations. You also need to remember which “home” (upstream) each version belongs to.
1 Like
I’m not here to defend the penguin but… I’m pretty sure we’ve been doing fork → PR all along. Even on the old Sangoma Bitbucket. “We” meaning third-party contributors/devs, not internal team. I would not expect external contributors working directly on the main repo.
Fork → PR is the standard rhythm of working on popular open source projects in Github. Which is not to say that it’s ideal. Working directly on the main repo could be allowed with Github’s controls requiring reviews and using actions to do some code style validations and such. But personally I think it’s ok to separate the corporate developers (working directly on the repo) and the rest of us riff-raff (working via forks).
One way I can think of to make it a little smoother is to set up a master repo for FreePBX and all of the components as linked submodules so that cloning brings the whole project down to your dev machine, then running the script that makes all the symlinks to /var/www/html/admin/modules/…
That is what the Development Environment is supposed to do (and used to) but it’s been a hot mess since everything at BitBucket was shut down. It was so that the community and internal developers were working from the same place. You could update/sync your system against all the modules and branches with a single command or update individual modules with current code with a single command.