-
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
Calico aws #282
Calico aws #282
Conversation
btw @atengler @katyafervent i doesnot remove anything, i just refactored methods queue |
kqueen/engines/jenkins.py
Outdated
@@ -64,6 +81,21 @@ def __init__(self, cluster, **kwargs): | |||
self.client = self._get_client() | |||
# Cache settings | |||
self.cache_timeout = 5 * 60 | |||
self.job_override_params = kwargs.get('override_parameters', {}) | |||
logger.critical('OVERRIDEDEEE {}'.format(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.
😅
kqueen/engines/jenkins.py
Outdated
logger.exception('Could not contact JenkinsEngine backend: ') | ||
status = config.get('PROVISIONER_ERROR_STATE') | ||
return status | ||
Check, that defined key parameter exist in Job |
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.
that defined key parameter exist in Job -> that key parameter, defined by the user exists in the specified Job
kqueen/engines/jenkins.py
Outdated
'timeout': 10 | ||
}) | ||
|
||
job_body = self.client.get_job_info(self.provision_job_name, depth=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.
I suggest to add try-catch here and return False from function end at catch block
@@ -118,6 +151,17 @@ def provision(self, **kwargs): | |||
# PATCH THE CTX TO CONTAIN CLUSTER NAME AND UUID | |||
ctx[cluster_name] = 'kqueen-{}'.format(self.cluster.id) | |||
ctx[cluster_uuid] = self.cluster.id | |||
if 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.
I suggest to manage all job parameters in a one place, so first let's add cluster_name and cluster_uuid to self.job_override_params (also for me self.job_parameters sounds better) and then manage everything in the same way
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.
imo its different cases - override-parameters used for overriding sane-default parameters, like boolean variables, checkers, additional cases
cluster name and UUID - must have cases, and it doesnot have any sane-defaults, so i prefer to manage it from separate UI-fields
kqueen/engines/jenkins.py
Outdated
@@ -118,6 +151,17 @@ def provision(self, **kwargs): | |||
# PATCH THE CTX TO CONTAIN CLUSTER NAME AND UUID | |||
ctx[cluster_name] = 'kqueen-{}'.format(self.cluster.id) | |||
ctx[cluster_uuid] = self.cluster.id | |||
if self.job_override_params: | |||
for key, value in 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.
self.job_override_params.items()
logger.error(msg) | ||
return False, msg | ||
|
||
logger.debug('Current Job configuration: {}, provisioning started...'.format(ctx)) |
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.
actually, it is started after 166 line
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.
there are try catch block, so i prefer to log here, its bad?
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 better to place it inside try-catch block
kqueen/engines/jenkins.py
Outdated
# TODO below form should be increased dynamically in case of 'add one more parameter' | ||
# Use-case: 'add parameter'->'key': 'value'-> 'add parameter'-> etc... | ||
# Should return dict{} | ||
'override_parameters': { |
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.
please, replace with
'override_parameters': {
'type': 'parameters',
'label': 'Jenkins job parameters',
'order': 2,
'validators': {
'required': False
}
}
},
'cluster': {}
}
Also, did you delete clusters occasionally?
Use cases: 1) Calico managing in case of using mcp-job calico-ha-aws-k8s 2) overriding parameters in case of any jenkins job From UI part: At least need to provide possibility of defining key(str):value(str) parameters
closed as duplicate |
scheme = https://www.lucidchart.com/invitations/accept/6e2bbc84-a5df-43f9-8b21-2a5b754a57d8