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

fix(notifications channel/destination update entity): Updated provider to work with new client version #2114

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

amaor-newrelic
Copy link
Contributor

@amaor-newrelic amaor-newrelic commented Nov 21, 2022

Description

Please include a summary of the change and which issue is fixed (if relevant).

Currently there's no way to update the boolean field "active" to be false, as the "omitempty" tag(?) parses false as empty. The change has been done in the client and it is a breaking change, so this PR was created to work with the new version

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

Please delete options that are not relevant.

  • My commit message follows conventional commits
  • My code is formatted to Go standards
  • I have performed a self-review of my own code
  • New and existing unit tests pass locally with my changes. Go here for instructions on running tests locally.

How to test this change?

Please describe how to test your changes. Include any relevant steps in the UI, HCL file(s), commands, etc

  • replace client in go mod with version from this pr
  • create a channel or destination
  • update the entity's active field to "false"
  • entity should update and no further changes will be pending when using terraform plan

@CLAassistant
Copy link

CLAassistant commented Nov 21, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 3 committers have signed the CLA.

✅ amaor-newrelic
❌ sanderblue
❌ mbazhlekova
You have signed the CLA already but the status is still pending? Let us recheck it.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@amaor-newrelic amaor-newrelic force-pushed the fix-channel-active-marshalling-issues branch from fbbe111 to 093a1e0 Compare November 29, 2022 13:14
@amaor-newrelic amaor-newrelic changed the title (fix): Updated provider to work with new client version fix(notifications channel/destination update entity): Updated provider to work with new client version Nov 29, 2022
@@ -18,9 +18,10 @@ func expandNotificationChannel(d *schema.ResourceData) notifications.AiNotificat
}

func expandNotificationChannelUpdate(d *schema.ResourceData) notifications.AiNotificationsChannelUpdate {
active := d.Get("active").(bool)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be nil if there is no active property, right?

channel := notifications.AiNotificationsChannelUpdate{
Name: d.Get("name").(string),
Active: d.Get("active").(bool),
Active: &active,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to upgrade the go-client version here?

@lzaga-newrelic
Copy link
Contributor

Also, looks like tests are failing :)

@sanderblue sanderblue force-pushed the main branch 2 times, most recently from e5e037d to af9e827 Compare January 18, 2023 21:14
@sanderblue sanderblue force-pushed the main branch 2 times, most recently from 23a73db to 4f7b20e Compare May 7, 2024 14:29
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.

5 participants