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

Can't use module on CWP platform #113

Closed
6 tasks done
robbieaverill opened this issue Jun 25, 2018 · 14 comments · Fixed by #114
Closed
6 tasks done

Can't use module on CWP platform #113

robbieaverill opened this issue Jun 25, 2018 · 14 comments · Fixed by #114

Comments

@robbieaverill
Copy link
Contributor

robbieaverill commented Jun 25, 2018

When trying to run the report generation task on the CWP platform we get this error in the failed job output:

[2018-06-25 15:03:04][INFO] Job caused exception The HOME or COMPOSER_HOME environment variable must be set for composer to run correctly in /sites/training-uat/releases/20180625025533/vendor/composer/composer/src/Composer/Factory.php at line 648

@sminnee suggested that he'd fixed the same error on the addons site by defining COMPOSE_HOME as one of your project variables in platform configuration, though for a default CWP site we'd rather define a fallback in code if it's not defined already, e.g.

if (!Environment::getEnv('COMPOSER_HOME')) {
    Environment::setEnv('COMPOSER_HOME', '/tmp');
}

Needs testing too of course

Pull requests

Tested on CWP and SSP

@sminnee
Copy link

sminnee commented Jun 25, 2018

Give it a god, but not sure if this will work as this would rely on putenv() to pass the variable to composer, which seems to fail intermittently.

@brynwhyman
Copy link

Is this issue relevant to SSP as well?

@rupachup rupachup added this to the Sprint 16 milestone Jun 25, 2018
@robbieaverill
Copy link
Contributor Author

Yep, just tested

@robbieaverill robbieaverill self-assigned this Jun 25, 2018
@robbieaverill
Copy link
Contributor Author

Environment::setEnv/putEnv doesn't carry through, but putenv works

@robbieaverill
Copy link
Contributor Author

Ok, now hitting a blocker with the CWP proxy (related: silverstripe/silverstripe-cms#2168).

Will need to see if we can inject this config somehow

@NightJar
Copy link
Contributor

Nice work!

@ScopeyNZ ScopeyNZ reopened this Jun 26, 2018
@robbieaverill
Copy link
Contributor Author

Need to backport change for the CWP 1.9 recipe too

@NightJar
Copy link
Contributor

NightJar commented Jun 26, 2018

I'd like to also point out that while we've achieved resolution in the context of which we are testing, I think this issue spans wider than just CWP (or the recipe of such at least), indicated in the fact that it also affected SSP - and while it works again now, it's because we're testing it in the context of a CWP recipe.

The fix:
https://github.com/silverstripe/cwp/blob/3ad235e1286939a5291a911c602ca42907b1746c/src/Extensions/MaintenanceProxyExtension.php#L29
Resides in cwp/cwp (where this extension also addresses platform specific proxy issues, which is a good thing).

Meaning anyone not using CWP (at least the module, but in the broader sense the platform as a whole) will still be affected by this exception any time they attempt to retrieve the task list (or run the task itself) from a user that does not have these environment variables set.

However if I were betting inclined, I'd wager that this will affect any web server that runs as it's own user - typically a non-login user meaning an absence any profile setup files... and in many a case it's a system user (UID lower than 1000) which can also mean there is no $HOME set either.

Exemplified in the virtual environment I was finally able to recreate this issue; there was nothing special about it's setup that I would consider unusual.

At this point I'd like to suggest moving this composer environment variable check (i.e. not the proxy ones from the same class) to that in which the exception manifests - bringyourownideas/silverstripe-composer-update-checker.

@robbieaverill
Copy link
Contributor Author

Sure, I'll do that

@NightJar
Copy link
Contributor

Ideal, all done! Thanks @robbieaverill

@robbieaverill
Copy link
Contributor Author

I've merged all the branches up.

@robbieaverill
Copy link
Contributor Author

Tested CWP 1.9 and 2.1 on CWP platform, all good!

@robbieaverill
Copy link
Contributor Author

Whoops, the task doesn't run in 1.9 on CWP

@robbieaverill
Copy link
Contributor Author

Falsy, forgot to deploy the updated 1.9 CWP code 🤦‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants