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

Provide an ordered and annotated config.yaml #334

Closed
wants to merge 5 commits into from

Conversation

wleese
Copy link
Contributor

@wleese wleese commented Dec 7, 2017

I found the config.yaml very, very hard to read.

Here's a copy of config.dev.yaml, with the following changes:

  • split into sections: api, sender and sync
  • annotate whatever I knew something of
  • add commented out configuration I hadn't seen mentioned elsewhere
  • consistently double # comments

Perhaps this belongs elsewhere. Just let me know.

Copy link
Contributor

@dwang159 dwang159 left a comment

Choose a reason for hiding this comment

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

Other than the missed TODO, looks awesome.


#oncall-api: https://oncall.dummy.com # oncall url

## TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I currently have a poor understanding of the purpose of these 'applications'. I'll remove the TODO for now and hope to add a comment in the future

# # Every $n users, stop inserting into mysql to avoid crashing it. Comment these out to disable.
# user_add_pause_interval: 500
# user_add_pause_duration: 1

Copy link
Member

@jrgp jrgp Dec 7, 2017

Choose a reason for hiding this comment

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

Should have a iris-owa-sync (outlook web access) block comment here


## Vendors provides a way to add support for new notification mechanisms
vendors: []
#- type: iris_slack
Copy link
Member

Choose a reason for hiding this comment

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

Should likely have the smtp example listed first (it seems missing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't seen any smtp configuration examples in the code base - did I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just took a quick look at the code and extracted the parameters.

# port: 8086
# database: iris

enable_gmail_oneclick: True
Copy link
Member

@jrgp jrgp Dec 7, 2017

Choose a reason for hiding this comment

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

Please change this default from True to False, and add a comment linking to https://developers.google.com/gmail/markup/reference/one-click-action

This functionality lets you claim incidents by clicking the link in gmail, which is very handy if you use g suite.

# name: email
# smtp_server: 'smtp1.example.com'
# smtp_gateway: 'smtp2.example.com'
# from: '[email protected]'
Copy link
Member

Choose a reason for hiding this comment

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

Can you add, double commented out, autoscale: False and tasks_per_mx: 4, with a comment describing how, if enabled, these will make a pool of persistent connections per each MX record for the given smtp_gateway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not entirely sure if I understand the double commented out correctly, but I've followed your instructions as literally as I could.

@houqp
Copy link
Contributor

houqp commented Dec 8, 2017

Any reason not to just replace config.dev.yaml with this? It's much more maintainable.

@wleese
Copy link
Contributor Author

wleese commented Dec 11, 2017

@houqp I'm fine with that too. @dwang159 @jrgp agreed?
#336

@houqp
Copy link
Contributor

houqp commented Dec 11, 2017

Easier to keep everything in sync if there is only one file to manage :)

@dwang159
Copy link
Contributor

dwang159 commented Jan 4, 2018

Closing in favor of #336

@dwang159 dwang159 closed this Jan 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants