Skip to content

Conversation

squat
Copy link
Contributor

@squat squat commented Feb 28, 2019

This commit removes the whitelist from the Telemeter server secret and
places it instead in a series of command line --whitelist flags. This
is helpful for several reasons:

  1. as the whitelist is no longer in a secret it no longer has to be
    handled manually, preventing potential human error when the whitelist
    changes; and
  2. when the whitelist changes, the DaemonSet spec will also change so
    the new configuration will automatically be rolled out without needing
    to manually kick pods.

cc @s-urbaniak @jfchevrette

This commit removes the whitelist from the Telemeter server secret and
places it instead in a series of command line `--whitelist` flags. This
is helpful for several reasons:
1. as the whitelist is no longer in a secret it no longer has to be
handled manually, preventing potential human error when the whitelist
changes; and
2. when the whitelist changes, the DaemonSet spec will also change so
the new configuration will automatically be rolled out without needing
to manually kick pods.
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 28, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2019
local secretVolume = volume.fromSecret(secretVolumeName, secretName);

local whitelist = std.map(
function(rule) "--whitelist='%s'" % std.strReplace(rule, 'ALERTS', 'alerts'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it make sense to do a std.asciiLower(str) here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no it does not make sense here; there may very well be metric names or labels in the whitelist that we do not want completely lowercased. The only one we care about today is the ALERTS metric name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

Copy link
Contributor

@s-urbaniak s-urbaniak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is super nice 👍 and will reduce skew. just one question regarding lowercasing vs. hardcoding the alert metric.

Copy link
Contributor

@s-urbaniak s-urbaniak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: s-urbaniak, squat

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@squat
Copy link
Contributor Author

squat commented Feb 28, 2019

/test e2e-aws

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@brancz
Copy link
Contributor

brancz commented Feb 28, 2019

/retest

@openshift-merge-robot openshift-merge-robot merged commit c0181c6 into openshift:master Feb 28, 2019
@squat squat deleted the whitelist branch March 20, 2019 15:33
@squat
Copy link
Contributor Author

squat commented Mar 20, 2019

fixes #121

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants