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

Non required parameters cause exception if missing #5

Open
cheribral opened this issue Apr 18, 2018 · 6 comments
Open

Non required parameters cause exception if missing #5

cheribral opened this issue Apr 18, 2018 · 6 comments

Comments

@cheribral
Copy link

It seems that non required parameters are actually required, since the agent will throw an exception if they are missing.

Adding a filter to parse_parameters fixes it for me:

--- /usr/lib/python2.7/site-packages/ocfagent/agent.py	2018-04-17 23:58:29.672919240 +0000
+++ -	2018-04-17 23:58:34.093745968 +0000
@@ -239,7 +239,7 @@
 	def parse_parameters(self):
 		"""Parse parameters (given with OCF_RESKEY_ prefix)"""
 		assert len(self.OCF_ENVIRON) > 0
-		for param_cls in self.parameter_spec:
+		for param_cls in [x for x in self.parameter_spec if x.required]:
 			cls_name = param_cls.name
 			env_name = "%s%s" % (OCF_RESKEY_PREFIX, cls_name)
 			if param_cls.type_def == types.IntType:  # nopep8

@cygnusb
Copy link
Contributor

cygnusb commented Apr 18, 2018

Yes, thanks. Looks fine

@cygnusb cygnusb closed this as completed Apr 18, 2018
@cygnusb cygnusb reopened this Apr 18, 2018
@cygnusb
Copy link
Contributor

cygnusb commented Apr 18, 2018

No, sorry. This will not correctly handle this. It will then not handle and check non-required parameters for the type of the parameter

@cheribral
Copy link
Author

Apologies, yes you're right. How is this instead?

--- /usr/lib/python2.7/site-packages/ocfagent/agent.py  2018-04-20 02:00:48.339014826 +0000
+++ -   2018-04-20 02:00:51.809065628 +0000
@@ -242,6 +242,8 @@
        for param_cls in self.parameter_spec:
            cls_name = param_cls.name
            env_name = "%s%s" % (OCF_RESKEY_PREFIX, cls_name)
+           if env_name not in self.OCF_ENVIRON:
+               continue
            if param_cls.type_def == types.IntType:  # nopep8
                param_cls.value = int(self.OCF_ENVIRON[env_name])
            elif param_cls.type_def == types.StringType:  # nopep8

@cygnusb
Copy link
Contributor

cygnusb commented Apr 20, 2018

This is not checking the required flag

@cheribral
Copy link
Author

I think my first suggestion for a fix might have confused the issue. This function was never checking for required attributes. A quick search through the file shows that required parameters are checked elsewhere.

My first suggestion was a bit brain dead, and used the required flag since those parameters were guaranteed to exist, so would not cause an exception. As you pointed out, that doesn't work if you need to parse non required parameters. It was a bit like fixing a squeaky wheel by removing the wheel.

The original and real issue of crashing on unpopulated environment variables is fixed by continuing the loop if they don't exist.

@cygnusb
Copy link
Contributor

cygnusb commented May 28, 2018

Please test this and provide a proper pull request

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

No branches or pull requests

2 participants