Here is my feedback on auth-proxy:
- install
--- As a subordinate, you should be careful when replacing files that could also be used by the main charm. I see that you are replacing default and default-ssl which may already have been modified and used by the main charm. At a minimum, you should determine if the file has been modified before replacing it. Arbitrarily replacing such a common ( and maybe essential ) file could be dangerous.
- stop
--- I see a similar problem with stopping apache2 altogether here ... As a subordinate charm, you don't know if apache is already being used by the main charm. Stopping it could stop the main charm as well.
- config-changed
--- I also see some of that here as well ....
All in all, I believe that the changes applied in this charm have to be applied very carefully and should be very targeted. A possible option would be to not assume that default and default-ssl are being used and allow the user to specify the name of the site being used ( you could default to default and default-ssl ). This could give you some indication of the site being used by the main charm.
I am marking the charm as Incomplete while you address the issues mentioned above. I would be happy to review the charm again when you're ready.
Please feel free to reach out to any of us in #juju on freenet ... we are here to help in any way we can.
Hi Paul:
Here is my feedback on auth-proxy:
- install
--- As a subordinate, you should be careful when replacing files that could also be used by the main charm. I see that you are replacing default and default-ssl which may already have been modified and used by the main charm. At a minimum, you should determine if the file has been modified before replacing it. Arbitrarily replacing such a common ( and maybe essential ) file could be dangerous.
- stop
--- I see a similar problem with stopping apache2 altogether here ... As a subordinate charm, you don't know if apache is already being used by the main charm. Stopping it could stop the main charm as well.
- config-changed
--- I also see some of that here as well ....
All in all, I believe that the changes applied in this charm have to be applied very carefully and should be very targeted. A possible option would be to not assume that default and default-ssl are being used and allow the user to specify the name of the site being used ( you could default to default and default-ssl ). This could give you some indication of the site being used by the main charm.
I am marking the charm as Incomplete while you address the issues mentioned above. I would be happy to review the charm again when you're ready.
Please feel free to reach out to any of us in #juju on freenet ... we are here to help in any way we can.
Thanks,
Juan