Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request] Improve Developer Onboarding / Workflow #1583

Closed
Ron-Zarbyte opened this issue Sep 7, 2023 · 12 comments · Fixed by #1692
Closed

[Feature Request] Improve Developer Onboarding / Workflow #1583

Ron-Zarbyte opened this issue Sep 7, 2023 · 12 comments · Fixed by #1692
Labels
documentation Improvements or additions to documentation feature request

Comments

@Ron-Zarbyte
Copy link
Contributor

Ron-Zarbyte commented Sep 7, 2023

Summary

Current onboarding and development workflow is a bit clunky and can be improved. There are a few aspects that get in the way of a seamless initial onboarding experience. This slows down the time it takes for a developer to be "onboarded" to the point of contributing code. This could also cause less experienced developers to shy away from attempting to contribute to FOSSBilling.

Optimizing development onboarding will help attract more developers to the project.

Goal

Developer onboarding should take just a few minutes, from the point the repo is cloned to the point of being able to code. The workflow should be entirely pre-configured and require little to no input from the developer. This also streamlines existing developer workflow configuration when moving to a new system.

Potential Solution

Implement workflow utilizing Docker to easily spin up a local development environment.

Technical Problems

These are the problems I've already ran in to attempting to create an optimized workflow. There may be something else I have not thought of.

Installation Folder Handling

When using docker to serve the git repo directly, the deletion of the install folder causes the changes to be detected in git as pending changes. This mixes in with other real changes you may be working on and becomes an annoyance. Here is an example:

image

The most straight forward workaround for this to me is to create a bypass within checkInstaller(). Perhaps we can create a new config.php setting which allows setting an option to bypass checking for the installation folder. This setting would default to false, and would only ever be changed to true for local development. An alternative would be environment variables, but config seems like a suitable place.

For example, a logic break within checkInstaller() such as the one described in the following image. This type of adjustment would be lightweight and not affect the existing behavior or security of the installation process for software end users.

image

Developing Locally with Docker

For the latest steps, refer to potential documentation updates on this branch.

Additional consideration may be needed to docker config. This is intended purely as a baseline to demonstrate the idea and to help generate feedback.

@BelleNottelling
Copy link
Member

The most straight forward workaround for this to me is to create a bypass within checkInstaller(). Perhaps we can create a new config.php setting which allows setting an option to bypass checking for the installation folder. This setting would default to false, and would only ever be changed to true for local development. An alternative would be environment variables, but config seems like a suitable place.

I for simplicity, I would likely pair that to the already existing debug param.

This is also potentially a bug that should be handled on a separate task, or a bug that is specific to environment, as I did not see recent new users of FOSSBilling running into this. Just me, seemingly...

It is a bug. You don't see users complaining about it because it's only in our dev branch and was introduced when I tightened the rules in our .htaccess to completely block access to all but specific PHP files. I was testing the changes on a installation that was already set up, so I never realized I forgot about the installer.

All that needs to happen to completely fix it is to edit the allow list and add in /install/index.php and /install/install.php. No biggie.

This can be temporarily bypassed by commenting the relevant line in the .htaccess

If this installation is at all accessible remotely, you need to reverse this ASAP. The existing .htaccess will work flawlessly after the installation and this line in it is responsible for preventing access to sensitive or dangerous files. Without it, your config file can be accessed through the browser and if someone can find a way to get a PHP script uploaded to it, RCE as well.

Regarding docker, we already maintain a docker image & config for it. I think our biggest issue for devs is almost entirely in the lack of documentation (which I have intended to fill out, but I have about a million things on my plate). If we can get the building steps for FOSSBilling documented pretty well and then also link to that repo as docker reference for if someone did want to use docker for development, I think that would pretty sufficiently handle that suggestion.

Thanks 😄

@Ron-Zarbyte
Copy link
Contributor Author

I for simplicity, I would likely pair that to the already existing debug param.

Got it. If I end up making some type of PR I will just use the existing debug flag for now.

It is a bug. You don't see users complaining about it because it's only in our dev branch and...

That makes sense. I only just recently started working with the project, so I was not sure if it was something I was doing differently. I will ignore that for now then. I've updated the original description to indicate it is an unrelated bug.

If this installation is at all accessible remotely...

No worries there. It is a local container, has web access but nothing inbound. I meant for it to come off as something to do for local development only. Sorry for any confusion on that.

Regarding docker, we already maintain a docker image & config for it. I think our biggest issue for devs is almost entirely in the lack of documentation (which I have intended to fill out, but I have about a million things on my plate). If we can get the building steps for FOSSBilling documented pretty well and then also link to that repo as docker reference for if someone did want to use docker for development, I think that would pretty sufficiently handle that suggestion.

I think here is where I can clarify a bit better.

This all originally stemmed from me reading this forum thread on setting up a development environment for the first time. I can see how that user was confused and I felt helping improve some of this is an ideal first step into the project, even if it ends up just being documentation improvement.

I admit I did overlook the docker repo. Although after looking at it, it appears to be intended for a production-like deployment or deploying a specific release version.

The use case I made a bad attempt at explaining would be for working with the repo and latest code base directly, versus deploying a container with a specific version of FOSSBilling inside. The perspective is a developer wanting to contribute code back to the project in the easiest way possible.

For example, here is the exact workflow I am picturing, start to finish, starting at reading documentation for the first time:

  1. Developer reads and meets system requirements from docs
  2. Developer clones/forks main FOSSBilling repository (depending on access, apparently)
  3. Open terminal/command line and cd to project root directory.
  4. Run command: composer install
  5. Run command: npm install
  6. Run command: npm run build
  7. Run command: docker-compose up -d --build

The developer would now have a fully functioning FOSSBilling application hosted locally. They would visit http://localhost/, follow the installation process, then they are able to freely made code edits while tracking changes in git. Task summary then addresses the new quirks from this, such as the installation folder tracking.

Allowing outside access at that stage is optional and would require extra steps. Outside access would of course be required to develop certain specific features, but this does get the application into a functional stage, sending emails, etc, which is where someone new to the project needs to be on day one.

This use case wouldn't require the external docker repo and would just require a similar docker compose config in the main repo (in the root directory, excluded from releases). It would be a slightly more ideal initial setup experience in my opinion.

However, I would not intend for this to replace the separate docker repo. I feel it is a different use case and that both are useful.

I am happy to try and clarify anything else. Feel free to also call me crazy in the event I am overlooking something.

@BelleNottelling
Copy link
Member

That makes sense. I only just recently started working with the project, so I was not sure if it was something I was doing differently. I will ignore that for now then. I've updated the original description to indicate it is an unrelated bug.

Just submitted a PR to correct this :)

No worries there. It is a local container, has web access but nothing inbound. I meant for it to come off as something to do for local development only. Sorry for any confusion on that.

I assumed it was, however when it comes to security I generally like to always point out when someone may be causing issues for themselves. Even if they are aware of the possible issues and have handled them, mentioning it helps ensure that someone else who's less knowledgeable won't blindly follow it and actually put their installation at risk.

Regarding the docker points, I definitely see the value in what you're suggestion. I'm just not sure if I love the idea of having docker related files in two separate repositories with similar, but slightly different purposes. I'm open to discussion on it and possible ways of keeping clean

Ron-Zarbyte added a commit to Ron-Zarbyte/FOSSBilling that referenced this issue Sep 11, 2023
Ron-Zarbyte added a commit to Ron-Zarbyte/FOSSBilling that referenced this issue Sep 11, 2023
@Ron-Zarbyte
Copy link
Contributor Author

Just submitted a PR to correct this :)

That fix is working good on my end now. Thanks! 👍

I assumed it was, however ...

I appreciate the thoroughness on security!

Regarding the docker points ... not sure if I love the idea of having docker related files in two separate repositories ...

I do agree about the duplicate configs. I am not sure of the cleanest way to solve that yet. I will need to think about that one more.

Worst case scenario, we could add Dockerfile and docker-compose.yml into the .gitignore for the main repo, and then we could list this process in docs as an optional workflow, along with listing the default docker file contents to use.

The ignore would keep staged changes clean for other devs who decide to use this method. It would avoid the duplicate config issue without having to somehow link the other repo or worry about future incompatible changes between the two use cases. What do you think about that?

I've commit my edits so far in my fork.

However, I did run into one more annoyance. It is regarding this section within install.php.

In my case I tried setting debug: true in the config-sample.php thinking it would get copied over as the default value. It of course did not, and with my adjustments this bug causes the install directory to still be deleted. If I restore the install folder and then set debug: true, my adjustments work as expected. I'd prefer to go ahead and address that too.

There is a TODO note here, but I assume it never got looped back to since this is such a low priority item that doesn't really affect much for most people. I want to go ahead and fix this by resolving the TODO note, but I wasn't sure if I should include it in my existing adjustments, or if you would prefer me to make a separate pull request for this. I'm OK with whatever your preference is.

I did not see an existing issue so we could perhaps just include it here.

Ron-Zarbyte added a commit to Ron-Zarbyte/FOSSBilling that referenced this issue Sep 11, 2023
@Ron-Zarbyte
Copy link
Contributor Author

I went ahead and created a separate branch in my fork which fixes carrying over default values from the sample config. Ref the commit above this comment.

It also greatly cleans up the existing config generation code. It still does not retain default comments but I don't think that is a concern worth the time to solve right now.

Let me know on if I should submit a PR for that separately or not. if not, I will merge it into my workflow branch with the other changes.

Ron-Zarbyte added a commit to Ron-Zarbyte/FOSSBilling that referenced this issue Sep 12, 2023
@Anuril
Copy link
Contributor

Anuril commented Sep 12, 2023

Great start, good job @Ron-Zarbyte!

I have two points that I would like to add:

  1. Using docker for development should not be the only documented way.
  2. One thing I see as crucial would be the test part for local development, as pushing to git to run the tests isn't very efficient.
    I've written up some stuff regarding the test environment before as I planned to improve the Onboarding as well. I'll get that to you so you can incorporate it if you want.

@Ron-Zarbyte
Copy link
Contributor Author

I made a rough draft for the workflow doc pages. You can find it here.

I need to clean up the docker method a bit more before I write the page for it. It is a bit verbose in some places because I tried to keep the perspective for someone new to github in general. I'm open to any suggestions.

Before any PR I would like to have the Docker, WSL, and Shared sections filled out enough to at least give a rough guide even if they are not perfect. The goal goes back to solving the perception confusion presented in this forum thread.

Ron-Zarbyte added a commit to Ron-Zarbyte/FOSSBilling that referenced this issue Sep 22, 2023
@Ron-Zarbyte
Copy link
Contributor Author

Ron-Zarbyte commented Sep 22, 2023

I've completed a more close to final version of the workflow for docker. It entirely uses Docker and makes no changes to or has any other requirements for the host OS.

I am now able to configure a dev environment, install fossbilling, make code changes, run unit tests, etc, from start to finish within the span of a few minutes. The only software you need installed on your local system is git, your code editor, and Docker Desktop.

You can find the latest articles at the links below.

DRAFT: Developer Workflow

DRAFT: Developer Workflow > Docker

I wrote the guide for having to manually copy in the docker build files, but I think it would be nice to still include the Dockerfile and docker-compose.yaml within the main repo itself alongside the other project configs (composer, npn, etc). This would help further optimize the onboarding experience and make the developer setup more seamless.

I don't think this conflicts with the separate docker repo because the use case is different. This use case is for development of the application, where the existing docker repo use case is for building a production-like deployment and not being concerned with git level code changes. Someone using the other docker repo won't have the full code base downloaded and will never see the docker configs from the main repo since they are outside of the src/ folder, so I do not personally see any conflict.

However if it is decided to keep it out of the repo, the configs can stay in docs, and we can add Dockerfile and docker-compose.yml to .gitignore to prevent a developer from having them showing in pending git changes.

@BelleNottelling when you have a moment I am curious of your thoughts on this with the latest updates.

Next I will be looking at documenting installation on a shared hosting provider (cPanel, Plesk, etc). I do agree with @Anuril that we should have more workflow options documented before committing to any doc updates. But I do think that after more people see this use case, more may lean towards using Docker versus other options like WSL.

@Anuril Anuril added the documentation Improvements or additions to documentation label Sep 27, 2023
@John-S4
Copy link
Member

John-S4 commented Oct 4, 2023

Really nice @Ron-Zarbyte thank you for the time that is going into this.

I just wanted to check, the end game here is to submit the dev install / workflow guides for both Docker and non-Docker to the FOSSBilling docs site, is that right?

I have recently roadmapped a major documentation update as a prerequisite before we can reach a stable 1.0 release (#1674}, and this would be a big help in adding some more useful empty into the quite lacking current documentation.

@Ron-Zarbyte
Copy link
Contributor Author

@John-S4 yeah that is correct. I added some of them as placeholders already but I did plan on writing actual pages for them. Work kept me busy the last couple weeks but I have more time again for this week. I'll get drafts in place for more workflows.

I am interested in helping expand the docs to solve #1674 after I finish this up, if it allows others to work on more meaningful code tasks. Still learning the codebase and working on the docs is a great way to get more familiar. Let me know the best way to coordinate with you on that, so I do not write pagers in my fork that someone else may be working on elsewhere.

The other area I wanted to look at was completely overhauling the installer. I accidentally started on it already out of annoyance, but I want to finish these workflow docs first.

@BelleNottelling
Copy link
Member

I am interested in helping expand the docs to solve #1674 after I finish this up, if it allows others to work on more meaningful code tasks. Still learning the codebase and working on the docs is a great way to get more familiar. Let me know the best way to coordinate with you on that, so I do not write pagers in my fork that someone else may be working on elsewhere.

It would be amazing if you could work on the docs. They'd benefit from the work & from everything I've seen you have genuinely good quality writing which is what we'd want on the docs.

As far as I know, I'm currently the only one with WIP changes for the docs and that's to document #1545 once it's down, so you shouldn't need to worry too much about doing conflicting work.

The other area I wanted to look at was completely overhauling the installer.

Please do, it's a bit of an old mess right now 😆

@Ron-Zarbyte
Copy link
Contributor Author

I updated the task description to remove the irrelevant info regarding the .htaccess fix.

I've also finished the workflow docs. I submit a pull request for review/final discussion of those changes.

ref FOSSBilling/fossbilling.org#154

Ron-Zarbyte added a commit to Ron-Zarbyte/FOSSBilling that referenced this issue Oct 4, 2023
This was referenced Oct 4, 2023
jaapmarcus pushed a commit that referenced this issue Oct 5, 2023
* #1583 initial workflow update using debug reference in config for install dir bypass

* #1583 add missed files

* #1583 improve config generation

* #1583 clean up docblock (issue feedback)

* fix https detection, corrected pull from upstream

* #1583 improve docker build config

* #1583 remove docker configs from repo, they have been moved to dev workflow documentation

* Two very minor changes

* Other minor changes

---------

Co-authored-by: Belle Aerni <[email protected]>
jaapmarcus pushed a commit that referenced this issue Oct 12, 2023
* #1583 initial workflow update using debug reference in config for install dir bypass

* #1583 add missed files

* #1583 improve config generation

* #1583 clean up docblock (issue feedback)

* fix https detection, corrected pull from upstream

* #1583 improve docker build config

* #1583 remove docker configs from repo, they have been moved to dev workflow documentation

* #1683 replace installer template assets

* #1683 initial UI layout overhaul, still need to port in some original UI logic

* #1683 initial backend overhaul, also still needs work

* #1683 misc cleanup, move license placement

* #1683 rename installer result template

* improve layout and content, fix variable naming #1683

* small fixes, readability improvement #1683

* stop passing references everywhere... use class variable instead #1683

* refactor unnecessary methods, improve more class variable references, optimize exception flow, remove duplicated method calls, verbose commenting/method renaming/code flow adjustment for readability #1683

* rename installer class (I think we deserve it at this point...) #1683

* improve form submission UX #1683

* block install if subfolder is detected #1683

* add validation for checking if we are already installed, secure against tampered requests #1683

* touch up install result for better onboarding #1683

* conver theme to jsdelivr, delete local picocss assets #1683

* fix typos #1683

* Use a local CSS file, other minor improvements

* Implemented new functionality on the install page

* Bug fix, make it more space efficient

* Minor fix

* Casing and minor JavaScript changes

* Adjusted the buttons

* Fix the blank screen issue

* window -> tab

* match other link tooltip adjustments

* update subfolder installations documentation link

* add Database prefix to db field labels

* Sentence case

* agreement checkbox

---------

Co-authored-by: Belle Aerni <[email protected]>
Co-authored-by: Yağızhan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants