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

Initial PR #1

Merged
merged 22 commits into from
Aug 7, 2020
Merged

Initial PR #1

merged 22 commits into from
Aug 7, 2020

Conversation

3h4x
Copy link

@3h4x 3h4x commented Aug 4, 2020

what

  • created opsgenie submodules
    • alert_policy
    • api_integration
    • escalation
    • notification_policy
    • team
    • team_routing_rule
  • every module is tested
  • every module has example
  • root module as an example module which use all submodules

notes

@cloudposse cloudposse deleted a comment from osterman Aug 5, 2020
@danjbh
Copy link
Contributor

danjbh commented Aug 5, 2020

I took a pass through all of the modules and spent a bit of time understanding each one -- it all looks pretty straight-forward to me!

@3h4x 3h4x marked this pull request as ready for review August 6, 2020 11:13
@3h4x 3h4x requested a review from osterman August 6, 2020 11:13
@3h4x
Copy link
Author

3h4x commented Aug 7, 2020

/test all

@3h4x
Copy link
Author

3h4x commented Aug 7, 2020

My main concern is variable naming. I found redundant to name variable alert_policy with added prefix opsgenie_ but maybe it would be preferred to do that. If such variable would be declared in external system I might find it confusing to see api_integration as not just opsgenie can provide it.
@osterman Any thoughts?

@3h4x 3h4x merged commit 6106246 into master Aug 7, 2020
@3h4x 3h4x deleted the initial branch August 7, 2020 17:11
@3h4x 3h4x restored the initial branch August 7, 2020 17:15
@3h4x 3h4x deleted the initial branch August 7, 2020 17:16
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.

3 participants