PR#81: install: server-install: abort installation on permission issue - ipsilon

ipsilon

#81 install: server-install: abort installation on permission issue

Merged 8 years ago by puiterwijk. Opened 8 years ago by puiterwijk.

install-perms-check into master

install: server-install: abort installation on permission issue

Patrick Uiterwijk • 8 years ago

a6a3dfa

ipsilon/install/ipsilon-server-install

file modified

+2 -3

		`@@ -41,7 +41,7 @@`
		`cherrypy.log.screen = False`

		`# Regular logging`
		`- LOGFILE = '/var/log/ipsilon-install.log'`
		`+ LOGFILE = os.environ.get('LOGFILE', '/var/log/ipsilon-install.log')`
		`logger = logging.getLogger()`


		`@@ -57,8 +57,7 @@`
		`try:`
		`lh = logging.FileHandler(LOGFILE)`
		`except IOError, e:`
		`- print >> sys.stderr, 'Unable to open %s (%s)' % (LOGFILE, str(e))`
		`- lh = logging.StreamHandler(sys.stderr)`
		`+ sys.exit('Unable to open %s (%s)' % (LOGFILE, str(e)))`
		`formatter = logging.Formatter('[%(asctime)s] %(message)s')`
		`lh.setFormatter(formatter)`
		`lh.setLevel(logging.DEBUG)`

tests/helpers/common.py

file modified

		`@@ -207,6 +207,11 @@`

		`def setup_idp_server(self, profile, name, addr, port, env):`
		`http_conf_file = self.setup_http(name, addr, port)`
		`+ logfile = os.path.join(self.testdir, name, 'logs', 'install.log')`
		`+ if env:`
		`+ env['LOGFILE'] = logfile`
		`+ else:`
		`+ env = {'LOGFILE': logfile}`
		`cmd = [os.path.join(self.rootdir,`
		`'ipsilon/install/ipsilon-server-install'),`
		`'--config-profile=%s' % profile]`

no initial comment

rcritten commented 8 years ago

Why not sys.exit(message)?

Should there be a rollback of things already done? I don't know, maybe not.

puiterwijk commented 8 years ago

Why not sys.exit(message)?
Because I was unaware of that version of sys.exit, will update the PR.

Should there be a rollback of things already done? I don't know, maybe not.
Well, openlogs() is called inside main(), before we even load the plugins, and way before we even parse any arguments.
At this point in the code, we have no output whatsoever yet.

puiterwijk commented 8 years ago

"output" being any change on the system.

pingou commented 8 years ago

Well, there is one output: Unable to open file...

That being said change looks fine to me nonetheless

rcritten commented 8 years ago

I would remove:

      print >> sys.stderr, 'Unable to open %s (%s)' % (LOGFILE, str(e))
      lh = logging.StreamHandler(sys.stderr)

And add:

     sys.exit('Unable to open %s (%s)' % (LOGFILE, str(e))

This will print the message to stderr and return 1.

puiterwijk commented 8 years ago

Right, so I realized that while this patch will fix that case, it will break the test suite, because that does not run as root.
The means that the test suite cannot write to the installer log file, which is perfectly fine since it can still continue the testrun.

I need to figure out a way to pass a different logging path without parsing the standard arguments, perhaps with environment variables. (we can't use standard arguments because those get parsed after we loaded all plugins, and we want to also add the loading of plugins to the installer log so we can't move the logging to before that).

rebased

8 years ago

puiterwijk commented 8 years ago

I just rebased this patch to use an environment variable to pass the logfile location.

The reason for this is that at the moment we open the logs, we have not yet parsed the arguments, and we couldn't even, because part of the logs are what plugins we have loaded.

pingou commented 8 years ago

Looks fine to me :)

rebased

8 years ago

Pull-Request has been merged by puiterwijk

8 years ago