-
Notifications
You must be signed in to change notification settings - Fork 16
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
Enable --cluster-monitoring flag description #48
Enable --cluster-monitoring flag description #48
Conversation
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
cc @tolusha |
@@ -129,9 +129,9 @@ installerString=" installer: string({\n\ | |||
clusterMonitoringString=" 'cluster-monitoring': boolean({\n\ | |||
default: false,\n\ | |||
hidden: false,\n\ | |||
description: \`Enable cluster monitoring to scrape metrics in Prometheus.\n\ | |||
description: \`Enable cluster monitoring to scrape CodeReady Workspaces metrics in Prometheus.\n\ |
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.
do we need to deviate from upstream here?
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 this case. We hide this option in chectl. That's why we have to readd in downstream.m
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.
so no need to inject "CodeReady Workspaces" into this line.
@@ -125,6 +126,13 @@ installerString=" installer: string({\n\ | |||
description: 'Installer type. If not set, default is "olm" for OpenShift >= 4.2, and "operator" for earlier versions.',\n\ | |||
options: ['olm', 'operator']\n\ | |||
}),"; # echo -e "$installerString" | |||
clusterMonitoringString=" 'cluster-monitoring': boolean({\n\ |
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.
for easier maintenance in future, this block could be read from upstream, then sed-replaced:
clusterMonitoringString="$(grep -A5 "'cluster-monitoring': boolean" src/commands/server/deploy.ts | sed -r -e "s/hidden: true/hidden: false/g")"; # echo -e "$clusterMonitoringString"
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.
@nickboldt How we spoke recently if I'll do in this way I'll have some problems by applying changes with sed but I have a lot of problems with sed.
I've tried for example
perl -0777 -p -i -e 's|(\ +'"'cluster-monitoring'"': boolean\({.*?}\),)| ${1} =~ /.+openshift.+/?"INSERT-CONTENT-HERE":${1}|gse' "${TARGETDIR}/${d}"
sed -r -e "s|INSERT-CONTENT-HERE|"\\\\ \\${clusterMonitoringString}$"|g" -i "${TARGETDIR}/${d}"
Can you help me please to find the right sed command?
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.
see inline comments
What does this PR do?
This PR enables cluster monitoring description in Readme.md. Also change default olm suggested namespace name(
openshift-workspaces
).Upstream related PR: che-incubator/chectl#966
What issues does this PR fix or reference?