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

docs: Update Test-Units.md #8707

Merged
merged 3 commits into from May 21, 2018

Conversation

Projects
None yet
4 participants
@mattie47
Copy link
Contributor

mattie47 commented May 13, 2018

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

@mattie47 mattie47 referenced this pull request May 13, 2018

Closed

Update Test-Units.md #8657

1 of 1 task complete

@kkrumm1 kkrumm1 changed the title Update Test-Units.md docs: Update Test-Units.md May 14, 2018

@murrant

This comment has been minimized.

Copy link
Member

murrant commented May 14, 2018

Seems to be a conflict there. one line says to install the system wide composer, the other executes ./composer.phar which is downloaded by the ./scripts/composer_wrapper.php script.

@laf

This comment has been minimized.

Copy link
Member

laf commented May 14, 2018

I'd prefer to get rid of suggesting to install composer by yum/apt.

@mattie47

This comment has been minimized.

Copy link
Contributor Author

mattie47 commented May 14, 2018

Seems to be a conflict there. one line says to install the system wide composer, the other executes ./composer.phar which is downloaded by the ./scripts/composer_wrapper.php script.

OK so I've gone off and done some googling...

In short:

  • composer is a package manager for php, similar to npm for node. It differs from things such as apt, or YUM in that packages can be installed on a per project basis.
  • apt install composer would globally install composer, which isn't necessarily needed...because:
    -- we can install composer.phar in the project folder instead. This is the binary file for per project managed dependencies.

In terms of LibreNMS

  • ./composer.json contains the list of requirements/dependencies for LibreNMS.
  • The reason we don't necessarily need composer installed globally, is because we have ./scripts/composer-wrapper.php
  • Composer-wrapper will attempt to download composer.phar into the libreNMS install directory
  • If Composer-wrapper can't download composer.phar, it will attempt to use a globally installed composer (if it is installed).
  • If all else fails, it'll tell the user to install composer.

What I don't understand is how composer-wrapper gets called in the first place? I thought perhaps through daily.sh, but this just seems to reference an already defined ${COMPOSER}...

I see from the LibreNMS install instructions one of the required packages is composer, which then uses composer --create-project to initially set the libreNMS project up. But beyond that, I'm not sure...

@laf

This comment has been minimized.

Copy link
Member

laf commented May 15, 2018

Forget what composer is or isn't outside of LibreNMS. The composer_wrapper script handles installing composer into /opt/librenms/ as composer.phar so we avoid older versions installed by the distro in the bin path, they've caused us problems before.

Once composer is available as /opt/librenms/composer.phar then composer_wrapper handles invoking it to install the dependancies we need.

@mattie47

This comment has been minimized.

Copy link
Contributor Author

mattie47 commented May 16, 2018

@laf,

OK - What do you actually want the docs here to say?

./scripts/composer_wrapper.php install ?

@laf

This comment has been minimized.

Copy link
Member

laf commented May 19, 2018

Correct.

mattie47 added some commits May 21, 2018

@laf laf merged commit 66ca9a7 into librenms:master May 21, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

mattie47 added a commit to mattie47/librenms that referenced this pull request Jul 2, 2018

Update Test-Units.md (librenms#8707)
DO NOT DELETE THIS TEXT

#### Please note

> Please read this information carefully. You can run `./scripts/pre-commit.php` to check your code before submitting.

- [x] Have you followed our [code guidelines?](http://docs.librenms.org/Developing/Code-Guidelines/)

#### Testers

If you would like to test this pull request then please run: `./scripts/github-apply <pr_id>`, i.e `./scripts/github-apply 5926`

@lock lock bot locked as resolved and limited conversation to collaborators Jul 20, 2018

@mattie47 mattie47 deleted the mattie47:patch-1 branch Sep 17, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.