Skip to content
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 config specs #7291

Merged
merged 8 commits into from
Aug 26, 2020
Merged

Add config specs #7291

merged 8 commits into from
Aug 26, 2020

Conversation

hithwen
Copy link
Contributor

@hithwen hithwen commented Aug 5, 2020

No description provided.

#
# health_service_check: true

## @param metrics - list of strings - optional
Copy link
Contributor Author

@hithwen hithwen Aug 5, 2020

Choose a reason for hiding this comment

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

I don't see this used in code but is not part of the openmetrics template either cc @ofek

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup only used by custom checks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? If our code is not reading this param then custom checks have to add its logic around it, right?
My question is if is safe to remove this param

#
# ssl_cert: "<CERT_PATH>"
Copy link
Contributor Author

@hithwen hithwen Aug 5, 2020

Choose a reason for hiding this comment

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

These are remapped on openmetrics base class (same for ssl_private_key and ssl_ca_cert)

Copy link
Contributor

@ruthnaebeck ruthnaebeck left a comment

Choose a reason for hiding this comment

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

docs review

amazon_msk/assets/configuration/spec.yaml Show resolved Hide resolved
## |__ aws_host
## |__ aws_service
##
## The `aws` auth type relies on boto3 to automatically gather AWS credentials (e.g. from `.aws/credentials`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## The `aws` auth type relies on boto3 to automatically gather AWS credentials (e.g. from `.aws/credentials`).
## The `aws` auth type relies on boto3 to automatically gather AWS credentials, for example: from `.aws/credentials`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is from the base template, see comments below

# password: <PASSWORD>

## @param ntlm_domain - string - optional
## If your services use NTLM authentication, you can specify
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## If your services use NTLM authentication, you can specify
## If your services use NTLM authentication, specify

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is from the base template, see comments below


## @param ntlm_domain - string - optional
## If your services use NTLM authentication, you can specify
## a domain that is used in the check. For NTLM Auth, append
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## a domain that is used in the check. For NTLM Auth, append
## the domain used in the check. For NTLM Auth, append

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is from the base template, see comment below


## @param kerberos_hostname - string - optional
## Override the hostname used for the Kerberos GSS exchange if its DNS name doesn't
## match its Kerberos hostname (e.g. behind a content switch or load balancer).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## match its Kerberos hostname (e.g. behind a content switch or load balancer).
## match its Kerberos hostname, for example: behind a content switch or load balancer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is on the base template. You guys may want to have a look at it and open a PR to it (it will update all affected integrations)

@hithwen hithwen mentioned this pull request Aug 6, 2020
@hithwen hithwen force-pushed the julia/amazon-msk-config-specs branch from 9076ec7 to e87cf77 Compare August 7, 2020 08:28
@hithwen hithwen requested a review from ruthnaebeck August 7, 2020 08:28
Copy link
Contributor

@ChristineTChen ChristineTChen left a comment

Choose a reason for hiding this comment

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

There is an openmetrics config template for init_config too

amazon_msk/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
@hithwen
Copy link
Contributor Author

hithwen commented Aug 20, 2020

@ruthnaebeck docs reviews (when adding gh suggestions) should be on the spec file not on the conf.yaml. conf.yaml is a generated file when a spec file is present.

@hithwen
Copy link
Contributor Author

hithwen commented Aug 20, 2020

There is an openmetrics config template for init_config too

added!

@hithwen hithwen requested a review from ofek August 20, 2020 12:37
@hithwen hithwen merged commit 03a7370 into master Aug 26, 2020
@hithwen hithwen deleted the julia/amazon-msk-config-specs branch August 26, 2020 08:41
github-actions bot pushed a commit that referenced this pull request Aug 26, 2020
* Add config specs

Co-authored-by: Christine Chen <[email protected]> 03a7370
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants