-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add override parameters to Jenkins #302
Conversation
kqueen/engines/jenkins.py
Outdated
'override_parameters': { | ||
'type': 'parameters', | ||
'label': 'Jenkins job parameters', | ||
'order': 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not required
kqueen/engines/jenkins.py
Outdated
'label': 'Jenkins job parameters', | ||
'order': 1, | ||
'validators': { | ||
'required': False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure it's needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, default is required
kqueen/engines/jenkins.py
Outdated
logger.exception('Could not contact JenkinsEngine backend: ') | ||
status = config.get('PROVISIONER_ERROR_STATE') | ||
return status | ||
Check, that key parameter, defined by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check that key parameter defined by
kqueen/engines/jenkins.py
Outdated
Returns: | ||
:obj:`jenkins.Jenkins`: initialized Jenkins client | ||
""" | ||
return jenkins.Jenkins(self.jenkins_url, **{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you pass it in a normal way?
kqueen/engines/jenkins.py
Outdated
try: | ||
job_body = self.client.get_job_info(self.provision_job_name, depth=1) | ||
for jj_class in job_body['property']: | ||
if 'parameterDefinitions' not in jj_class.keys(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not in jj_class:
?
kqueen/engines/jenkins.py
Outdated
if self.job_override_params: | ||
logger.debug('The following job parameters will be overwritten: {}' | ||
.format(self.job_override_params)) | ||
for key, value in self.job_override_params.items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ctx.update(self.job_override_params)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried this? It should work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG mostly
just a few comments
re-trigger travis with amend
and do u want to push into master directly or firstly push to my PR?
@@ -227,7 +227,7 @@ def dispatch_request(self, *args, **kwargs): | |||
self.save_object() | |||
self.after_save() | |||
except Exception as e: | |||
current_app.logger.error(e) | |||
current_app.logger.exception(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e is redundant, coz it in that way traceback will be duplicated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not in that case this is general logging, we don't log exceptions everywhere so this is the one way to get info about exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, okok
kqueen/engines/jenkins.py
Outdated
|
||
Returns: | ||
:obj:`jenkins.Jenkins`: initialized Jenkins client | ||
bool: True if all parameters exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove all
this method takes 1 arg
f79520f
to
a710908
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGFM
a710908
to
340827a
Compare
if auth_verify: | ||
status = config.get('PROVISIONER_OK_STATE') | ||
except Exception as e: | ||
logger.exception('Could not contact JenkinsEngine backend: ') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like smth is missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just code movement, not a scope of this commit
Use cases: 1) Calico managing in case of using mcp-job calico-ha-aws-k8s 2) Overriding parameters in case of any jenkins job
340827a
to
3f88bae
Compare
No description provided.