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

new: add 'emails' attribute to Alarm spec #310

Merged
merged 1 commit into from May 7, 2019

Conversation

Projects
None yet
4 participants
@aaslamin
Copy link
Contributor

commented Apr 30, 2019

Issue: aporeto-inc/aporeto#1284

tl;dr we want to avoid spamming the namespace owner(s) with emails every time a new alarm is created. We will accomplish this by extending the alarms API such that it accepts a list of emails that should be notified when an alarm is created.

Next up is modifying the service that handles the alarms API (sephiroth i think?) to consume this new field and send the email to the intended recipients; as a result we will no longer traverse the ancestors of the namespace to fetch each of their respective owner emails.

@aaslamin aaslamin requested review from primalmotion and t00f Apr 30, 2019

@@ -78,3 +78,12 @@ attributes:
- Resolved
default_value: Open
orderable: true

- name: emails

This comment has been minimized.

Copy link
@primalmotion

primalmotion Apr 30, 2019

Member

please use the VSCode extension is you use VSCode, or run rego format to format the yaml

This comment has been minimized.

Copy link
@primalmotion

primalmotion Apr 30, 2019

Member

this also needs to be stored: true

This comment has been minimized.

Copy link
@aaslamin

aaslamin Apr 30, 2019

Author Contributor

Done

alarm.go Outdated
@@ -121,6 +121,9 @@ type Alarm struct {
// Description is the description of the object.
Description string `json:"description" msgpack:"description" bson:"description" mapstructure:"description,omitempty"`

// List of recipients that should be emailed when this alarm is created.

This comment has been minimized.

Copy link
@t00f

t00f Apr 30, 2019

Member

See my comment below.

@@ -78,3 +78,12 @@ attributes:
- Resolved
default_value: Open
orderable: true

- name: emails
description: List of recipients that should be emailed when this alarm is created.

This comment has been minimized.

Copy link
@t00f

t00f Apr 30, 2019

Member

Description should start with the name and uppercase. Otherwise it is not a valid Golang description (See comment above)

This comment has been minimized.

Copy link
@t00f

t00f Apr 30, 2019

Member

In this case:

Emails is the list of recipients...

This comment has been minimized.

Copy link
@aaslamin

aaslamin Apr 30, 2019

Author Contributor

Done

This comment has been minimized.

Copy link
@primalmotion

primalmotion Apr 30, 2019

Member

actually no this is wrong. Golang doesn't care about the format of the comment on variables.
Making the description like that makes super weird documentation

This comment has been minimized.

Copy link
@aaslamin

aaslamin Apr 30, 2019

Author Contributor

Maybe @t00f was referring to this style guide @primalmotion?

https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences

Comments should begin with the name of the thing being described and end in a period.

This comment has been minimized.

Copy link
@primalmotion

primalmotion Apr 30, 2019

Member

Yes. But the primary goal of this is to have good apidoc :) in any case it's fine

This comment has been minimized.

Copy link
@t00f

t00f Apr 30, 2019

Member

We always followed that rules: Documentation might be weird but consistent :)

@aaslamin aaslamin force-pushed the aporeto-1284 branch 3 times, most recently from 304a4b9 to cb18178 Apr 30, 2019

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Alrighty, I think I am ready for another looksy s'il vous plaît 🙏

@t00f
Copy link
Member

left a comment

Looks good to me 👍

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

What do I need to do for the unit tests to run in CI? It seems to be stuck in yellow/waiting 🤷‍♂

@t00f

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

You can add a single comment that says /build here

@t00f

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

/build

@codecov

This comment has been minimized.

Copy link

commented Apr 30, 2019

Codecov Report

Merging #310 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #310      +/-   ##
==========================================
- Coverage   10.79%   10.79%   -0.01%     
==========================================
  Files         120      120              
  Lines       35259    35269      +10     
==========================================
  Hits         3806     3806              
- Misses      31445    31455      +10     
  Partials        8        8
Impacted Files Coverage Δ
alarm.go 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d51c4b...6d0bae8. Read the comment docs.

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@primalmotion @t00f if this looks A-OK to you, then I will :shipit:

@primalmotion

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

don't merge in before the backend implementation is ready

@t00f

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

@aaslamin you can use a github project to gather multiple PRs. See https://github.com/orgs/aporeto-inc/projects

@primalmotion

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

the way we do, is to open a project here https://github.com/orgs/aporeto-inc/projects

  • add all the PR containing changes
  • add integration tests
  • when ready go to slack and ask HAL9000 build <name-of-the-project>. This will build the backend by replacing all libraries with the versions of the ones that are part of the project
  • HAL9000 test <name-of-the-project>: this will run apotests (functional tests) to ensure we did not break anything (you'll learn about apotests....)

When everything is green, we merge

@primalmotion

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

(this is of course overkill for something that small, but let's start correctly :) )

@aporeto-bot

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "issue-1284",
    "component": "gaia",
    "pr-id": "310",
    "commit-sha": "cb18178fa4343c3135a996086a5091f62695e9e8"
  },
  {
    "project": "issue-1284",
    "component": "backend",
    "pr-id": "383",
    "commit-sha": "e3664ad206dcb11e50ab3cf5ce4bb00b87fe419a"
  }
]
@aporeto-bot

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "issue-1284",
    "component": "gaia",
    "pr-id": "310",
    "commit-sha": "cb18178fa4343c3135a996086a5091f62695e9e8"
  },
  {
    "project": "issue-1284",
    "component": "backend",
    "pr-id": "383",
    "commit-sha": "25503f5300a15b60798bf5a0bc290e622ec8765a"
  }
]
@aporeto-bot

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "issue-1284",
    "component": "gaia",
    "pr-id": "310",
    "commit-sha": "cb18178fa4343c3135a996086a5091f62695e9e8"
  },
  {
    "project": "issue-1284",
    "component": "backend",
    "pr-id": "383",
    "commit-sha": "c35a560059ca61b001cdfde8af883eb6d1d23ed7"
  }
]

@aaslamin aaslamin dismissed stale reviews from primalmotion and t00f via 6d0bae8 May 2, 2019

@aaslamin aaslamin force-pushed the aporeto-1284 branch from cb18178 to 6d0bae8 May 2, 2019

@aporeto-bot

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "issue-1284",
    "component": "gaia",
    "pr-id": "310",
    "commit-sha": "6d0bae864631640fcffb763e40d09b2d1fb4878e"
  },
  {
    "project": "issue-1284",
    "component": "backend",
    "pr-id": "383",
    "commit-sha": "6bcff2b531a626a076768f3ae120c049be902310"
  }
]
@aporeto-bot

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "issue-1284",
    "component": "gaia",
    "pr-id": "310",
    "commit-sha": "6d0bae864631640fcffb763e40d09b2d1fb4878e"
  },
  {
    "project": "issue-1284",
    "component": "backend",
    "pr-id": "383",
    "commit-sha": "7689013d6c1308f8293554b5379402d870b9f12a"
  }
]
@aporeto-bot

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "issue-1284",
    "component": "gaia",
    "pr-id": "310",
    "commit-sha": "6d0bae864631640fcffb763e40d09b2d1fb4878e"
  },
  {
    "project": "issue-1284",
    "component": "backend",
    "pr-id": "383",
    "commit-sha": "6d838d7bca0d3f36d35bb2c1749f70e14a454c27"
  }
]
@aporeto-bot

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "issue-1284",
    "component": "gaia",
    "pr-id": "310",
    "commit-sha": "6d0bae864631640fcffb763e40d09b2d1fb4878e"
  },
  {
    "project": "issue-1284",
    "component": "backend",
    "pr-id": "383",
    "commit-sha": "9b848dbb39e4a2b4284724b40579a99463c9755f"
  }
]
@aporeto-bot

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "issue-1284",
    "component": "gaia",
    "pr-id": "310",
    "commit-sha": "6d0bae864631640fcffb763e40d09b2d1fb4878e"
  },
  {
    "project": "issue-1284",
    "component": "backend",
    "pr-id": "383",
    "commit-sha": "9b848dbb39e4a2b4284724b40579a99463c9755f"
  }
]
@aporeto-bot

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "issue-1284",
    "component": "gaia",
    "pr-id": "310",
    "commit-sha": "6d0bae864631640fcffb763e40d09b2d1fb4878e"
  },
  {
    "project": "issue-1284",
    "component": "backend",
    "pr-id": "383",
    "commit-sha": "5ba0c264b85ea50444c8a999a5a7cef922a9b58f"
  }
]
@aporeto-bot

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "issue-1284",
    "component": "gaia",
    "pr-id": "310",
    "commit-sha": "6d0bae864631640fcffb763e40d09b2d1fb4878e"
  },
  {
    "project": "issue-1284",
    "component": "backend",
    "pr-id": "383",
    "commit-sha": "3216247edfb514f51a2be1b2fd4d0b86d1b557c6"
  }
]

@primalmotion primalmotion merged commit f47e652 into master May 7, 2019

4 of 6 checks passed

codecov/patch 0% of diff hit (target 10.79%)
Details
codecov/project 10.79% (-0.01%) compared to 1d51c4b
Details
built
Details
functional-tests Submitter: reason: . functional-tests set to success
Details
functional-tests-trigger Submitter: reason: . functional-tests-trigger set to success
Details
unit-tests
Details

@primalmotion primalmotion deleted the aporeto-1284 branch May 7, 2019

CyrilPeponnet added a commit that referenced this pull request May 7, 2019

Background index (#316)
* new: add 'emails' attribute to Alarm for aporeto-inc/aporeto#1284 (#310)

* Fixed: Expand when writing to doc file and update README (#314)

* Update: Make document generation part of make codegen and update README

* Fixed: Correct command from .regolithe-gen-cmd

* new: index hojo activity in background
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.