-
Notifications
You must be signed in to change notification settings - Fork 45
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
Template and store replicas count as service configuration in Metalk8s cluster #2258
Template and store replicas count as service configuration in Metalk8s cluster #2258
Conversation
Hello ebaneck,My role is to assist you with the merge of this Status report is not available. |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option
|
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following reviewers are expecting changes from the author, or must review again: |
import logging | ||
import re | ||
|
||
from salt.exceptions import CommandExecutionError |
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 respect pylint import suggestion
C: 8, 0: third party import "import yaml" should be placed before "from salt.exceptions import CommandExecutionError" (wrong-import-order)
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.
Maybe we should setup pylint for Salt tree (or _modules
at least)?
That would automatically flag unused import and import ordering.
That way we can enforce it in CI and focus our brain's bandwidth on functional issues.
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.
Agree we should but we need some extra stuff about "salt" stuffs like all the __opts__
__salt__
__pillar__
... but anyway agree we should have salt module checking in the CI so linting + unit test
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.
If this is something we really want to enforce, it would make sense to add it in CI. Especially since not everyone here uses pylint
+ the special Salt plugin to lint their Salt modules (I don't).
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 thin there is a pylint plugin for Salt, otherwise we can simply ignore Salt stuff by tweaking Pylint conf (you can probably consider some stuff as blackbox).
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.
https://github.com/saltstack/salt-pylint (Personnaly never tested)
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 think @jbertran tried it (but I may remember wrong)
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following reviewers are expecting changes from the author, or must review again: |
c1b9b89
to
2afbfcf
Compare
dd39d83
to
e5c6401
Compare
salt/metalk8s/addons/prometheus-operator/deployed/service-configuration.sls
Outdated
Show resolved
Hide resolved
c4bf2ef
to
71257ae
Compare
71257ae
to
0d02d6e
Compare
da93185
to
fb7a2f4
Compare
fb7a2f4
to
d4aa36c
Compare
salt/metalk8s/addons/prometheus-operator/deployed/service-configuration.sls
Show resolved
Hide resolved
…uster Initially, we do not deploy and manage service configurations using configmaps. This commit adds generic configmaps with service related configurations such that the end user can configure cluster settings directly by editing the related configmaps
This commit adds a salt module used to query a configmap object within a metalk8s cluster Once called with a service config parameter, this module returns the value of the configuration
This commit adds argparse to our render script and introduces a new argument `--service-config` which can be called several times accepting only two arguments(service name & service configmap name)
This hack is required to bypass the fact that dumping Jinja to YAML does not work with the render script because of the presence of unquote special characters e.g `{%`. If we try to escape thess special characters, we end up with type(str) always and in some special conditions, we need for example replicas count to be of type(int)
The prometheus-operator chart is rendered using the following command: ./charts/render.py prometheus-operator --namespace metalk8s-monitoring charts/prometheus-operator.yaml --service-config grafana metalk8s-grafana-config --service-config prometheus metalk8s-prometheus-config --service-config alertmanager metalk8s-alertmanager-config charts/prometheus-operator/ > salt/metalk8s/addons/prometheus-operator/deployed/chart.sls Service configuration values are passed using `--service-config` argument. When rendering this chart, we must pass the following services(grafana, prometheus, alertmanager) including the configmap that holds their respective service & cluster configurations Closes: #2255
d4aa36c
to
6f96f83
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.
LGTM
/approve |
/reset |
Reset completeI have successfully deleted this pull request's integration branches. The following options are set: approve |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option
The following options are set: approve |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following reviewers are expecting changes from the author, or must review again: The following options are set: approve |
1 similar comment
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following reviewers are expecting changes from the author, or must review again: The following options are set: approve |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following reviewers are expecting changes from the author, or must review again: The following options are set: approve |
/status |
StatusStatus report is not available. The following options are set: approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue None. Goodbye ebaneck. |
Component:
'salt', 'charts',
Context:
See #2255
Summary:
Template and render the charts to consume service configuration values directly.
Acceptance criteria:
Closes: #2255