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

jiralert: Improved docs and config. #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

bwplotka
Copy link
Member

@bwplotka bwplotka commented Apr 12, 2019

Changes:

  • Template file is not needed anymore if you don't define anything. (Remove need to template file #11)
  • Clean up of not used structs & vars
  • Improved comments on doc which makes example not needed (if we can use goDocs)

Thoughts @free ? (:

Signed-off-by: Bartek Plotka [email protected]

@bwplotka bwplotka requested a review from free April 12, 2019 20:29
Copy link
Member

@free free left a comment

Choose a reason for hiding this comment

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

Lots of nitpicking (apologies for that) and a more general comment: I (and I'm sure lots of others) find it really useful to have a sample configuration file as a starting point. If I was a Java-only (or Python-only or whatever) developer I'd find it really inconvenient to parse Golang documentation in order to extract a working example for a basic configuration.

This is even worse regarding the removal of the template file: we are now asking people who may not even be coders to figure out Golang's template language plus Alertmanager's data structures in order to write a bunch of templates, unless they are fine with all their tickets being named "Alert" and having an empty description.

At the very least, the sample configuration and template file contents should be included verbatim into the documentation. (Even though, as you noticed, said documentation can disappear without trace for weeks at a time from GoDoc.)

Summary string `yaml:"summary" json:"summary"`
// ReopenState specifies the state to transition into when reopening a closed issue.
// This state has to exists for the chosen project.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// This state has to exists for the chosen project.
// This state must exist for the chosen project and should ideally be reachable from all other states.

You can find more docs in [the configuration itself](/pkg/config/config.go)

Each receiver must have:
* a unique name (matching the Alertmanager receiver name)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* a unique name (matching the Alertmanager receiver name)
* a unique name (matching the Alertmanager receiver name),

Each receiver must have:
* a unique name (matching the Alertmanager receiver name)
* JIRA API access fields (URL, username and password),
* handful of required issue fields (such as the JIRA project and issue summary),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* handful of required issue fields (such as the JIRA project and issue summary),
* required issue fields (such as the JIRA project and issue summary),

* a unique name (matching the Alertmanager receiver name)
* JIRA API access fields (URL, username and password),
* handful of required issue fields (such as the JIRA project and issue summary),
* some optional issue fields (e.g. priority) and a `fields` map for other (standard or custom) JIRA fields.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* some optional issue fields (e.g. priority) and a `fields` map for other (standard or custom) JIRA fields.
* optional issue fields (e.g. priority), including a `fields` map for other (standard or custom) JIRA fields.

type ReceiverConfig struct {
// Name represents unique name for a receiver.
// If Iiralert is used with Alertmanager, name it as Alertmanager receiver that sends alert via webhook to Jiralert for
// desired propagation.
Copy link
Member

Choose a reason for hiding this comment

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

I'd replace all this with

	// Name is a unique receiver name. It must match an Alertmanager receiver name. Required.

Priority string `yaml:"priority" json:"priority"`
// Description specifies Golang template invocation for generating the issue description.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Description specifies Golang template invocation for generating the issue description.
// Description is a plain string; or Golang template invocation to generate the issue description.

WontFixResolution string `yaml:"wont_fix_resolution" json:"wont_fix_resolution"`
// Fields specifies standard or custom field values to set on created issue.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Fields specifies standard or custom field values to set on created issue.
// Fields specifies standard or custom field values to set on created issue. E.g.
// * TextField: "Random text"
// * SelectList: { "value": "red" }
// * MultiSelect: [{"value": "red" }, {"value": "blue" }, {"value": "green" }]

@@ -123,8 +144,13 @@ func (rc *ReceiverConfig) UnmarshalYAML(unmarshal func(interface{}) error) error

// Config is the top-level configuration for JIRAlert's config file.
type Config struct {
// Default specifies default values to be used in place of any ReceiverConfig' empty field.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Default specifies default values to be used in place of any ReceiverConfig' empty field.
// Default specifies default values to be used in place of any ReceiverConfig empty field.

}
return &Template{tmpl: tmpl}, nil

level.Info(logger).Log("msg", "no template was passed.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
level.Info(logger).Log("msg", "no template was passed.")
level.Info(logger).Log("msg", "no template file provided")

Receivers []*ReceiverConfig `yaml:"receivers,omitempty" json:"receivers,omitempty"`

// Template specifies an optional file with template definitions.
Copy link
Member

Choose a reason for hiding this comment

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

I have an issue with defaulting to no template file: with no templating, JIRAlert will always create identical JIRA tickets (except the label, but that's not even intended for human consumption). Why would anyone want that at all? Much less as default behavior?

(I know about the deployment issue someone brought up, but I have a hard time believing there exists a widely used deployment tool that trips up on Golang templates and provides no workaround for that. What if the binary contains Golang templates as strings?)

@bwplotka
Copy link
Member Author

bwplotka commented Mar 5, 2021

Let me get back to this, rebase and improve 🤗

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.

2 participants