Skip to content
This repository has been archived by the owner on Jun 2, 2024. It is now read-only.

add data source alerting channel office365 #151

Merged
merged 7 commits into from
Jul 29, 2023
Merged

Conversation

rumenvasilev
Copy link
Contributor

@rumenvasilev rumenvasilev commented Jul 26, 2023

I would like to add this data source. Implementation-wise I've confirmed it works with terraform code and instana server. Documentation is added too.
Sadly I had to copy the implementation for AlertingChannels to AlertingChannelsDS, due to the fact the former is used as pointer receiver, while we need a value receiver for the read-only interface implementation currently in use for the other data sources.

@rumenvasilev
Copy link
Contributor Author

I've prepared a better DRY version of the alert channel code, but will push it tomorrow after I validate it functions properly. If you don't insist on a DRY-er version for now, I'll keep it for a different PR.

@gessnerfl
Copy link
Owner

@rumenvasilev I would prefer a DRY-er version. I you don't feel comfortable I would be ok with the change, merge it into a feature branch and cross check options to clean up.

@rumenvasilev
Copy link
Contributor Author

@rumenvasilev I would prefer a DRY-er version. I you don't feel comfortable I would be ok with the change, merge it into a feature branch and cross check options to clean up.

Here's a more compact version. Tested, still works as before.
The only missing piece I couldn't find is if there's any limitation of characters and length of alert channel name. Instana API doesn't mention that in their docs, hence why I left the comment in the code.

Type: schema.TypeString,
Required: true,
Description: "The name of the alerting channel",
// TODO: What is the max length here?
Copy link
Owner

Choose a reason for hiding this comment

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

No restriction defined in the API specification. Would skip also not add any validation in this case.

@gessnerfl gessnerfl changed the base branch from main to feature/datasource-alert-config-o365 July 29, 2023 17:43
@gessnerfl gessnerfl merged commit 01228dc into gessnerfl:feature/datasource-alert-config-o365 Jul 29, 2023
3 of 4 checks passed
@gessnerfl
Copy link
Owner

@rumenvasilev thanks a lot for your contribution. I will take it from here and finalize the change on a feature branch

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants