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(deps): update module github.com/atc0005/go-teams-notify/v2 to v2.7.0 #488

Closed

Conversation

renovate[bot]
Copy link
Contributor

@renovate renovate bot commented Dec 12, 2022

Mend Renovate

This PR contains the following updates:

Package Type Update Change
github.com/atc0005/go-teams-notify/v2 require minor v2.6.1 -> v2.7.0

Release Notes

atc0005/go-teams-notify

v2.7.0

Compare Source

Added
  • (GH-134) Allow setting user agent, fallback to project-specific default
    value
  • (GH-135) Allow overriding default http.Client
  • (GH-157) Add Adaptive Card message format support
  • (GH-169) Added YAML en(de)coding support to MessageCard
Changed
  • Dependencies
    • github.com/stretchr/testify
      • v1.7.0 to v1.8.1
  • (GH-154) Deprecate API interface, expose underlying "Teams" client
  • (GH-183) Update Makefile and GitHub Actions Workflows
  • (GH-190) Refactor GitHub Actions workflows to import logic
Fixed
  • (GH-166) Update lintinstall Makefile recipe
  • (GH-184) Apply Go 1.19 specific doc comments linting fixes
  • (GH-176) ./send_test.go:238:8: second argument to errors.As should not be *error
  • (GH-179) Wrong json key name for URL (uses uri instead)

Configuration

📅 Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 Automerge: Enabled.

Rebasing: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 Ignore: Close this PR and you won't be reminded about this update again.


  • If you want to rebase/retry this PR, check this box

This PR has been generated by Mend Renovate. View repository job log here.

@renovate renovate bot added the type/deps label Dec 12, 2022
@renovate renovate bot force-pushed the renovate/github.com-atc0005-go-teams-notify-v2-2.x branch from f6ac149 to 900aedd Compare December 15, 2022 15:13
@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2022

Codecov Report

Patch and project coverage have no change.

Comparison is base (6ad4b13) 75.07% compared to head (ccf0aa0) 75.07%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #488   +/-   ##
=======================================
  Coverage   75.07%   75.07%           
=======================================
  Files          41       41           
  Lines        1376     1376           
=======================================
  Hits         1033     1033           
  Misses        251      251           
  Partials       92       92           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@renovate renovate bot force-pushed the renovate/github.com-atc0005-go-teams-notify-v2-2.x branch 6 times, most recently from 3b4045d to 26f4227 Compare December 16, 2022 20:21
@renovate renovate bot changed the title fix(deps): update module github.com/atc0005/go-teams-notify/v2 to v2.7.0 Update module github.com/atc0005/go-teams-notify/v2 to v2.7.0 Dec 17, 2022
@renovate renovate bot changed the title Update module github.com/atc0005/go-teams-notify/v2 to v2.7.0 fix(deps): update module github.com/atc0005/go-teams-notify/v2 to v2.7.0 Dec 17, 2022
@renovate renovate bot force-pushed the renovate/github.com-atc0005-go-teams-notify-v2-2.x branch 4 times, most recently from 3393982 to 7eb0013 Compare December 21, 2022 00:02
@renovate renovate bot force-pushed the renovate/github.com-atc0005-go-teams-notify-v2-2.x branch 4 times, most recently from 1a4e511 to bcfc602 Compare January 6, 2023 04:25
@renovate renovate bot force-pushed the renovate/github.com-atc0005-go-teams-notify-v2-2.x branch 3 times, most recently from 512beda to 58cdf38 Compare January 17, 2023 13:45
@renovate renovate bot force-pushed the renovate/github.com-atc0005-go-teams-notify-v2-2.x branch from 58cdf38 to 424142c Compare January 18, 2023 15:34
@renovate renovate bot force-pushed the renovate/github.com-atc0005-go-teams-notify-v2-2.x branch from 424142c to 5116cfa Compare January 19, 2023 07:59
@renovate renovate bot force-pushed the renovate/github.com-atc0005-go-teams-notify-v2-2.x branch 6 times, most recently from 892ade1 to 73db619 Compare March 1, 2023 03:09
@renovate renovate bot force-pushed the renovate/github.com-atc0005-go-teams-notify-v2-2.x branch 5 times, most recently from 4237688 to 0d73d95 Compare March 11, 2023 07:05
@renovate renovate bot force-pushed the renovate/github.com-atc0005-go-teams-notify-v2-2.x branch 5 times, most recently from b37a8dc to d7e6d39 Compare March 15, 2023 21:40
@atc0005
Copy link

atc0005 commented Mar 16, 2023

The external library recently introduced some breaking changes

Hi @nikoksr,

Is this a reference to the following GH issue?

I completely understand the desire to drop external dependencies, but wanted to touch base to make sure I hadn't missed something else.

Thanks in advance.

@renovate renovate bot force-pushed the renovate/github.com-atc0005-go-teams-notify-v2-2.x branch 5 times, most recently from 5999f0e to ccf0aa0 Compare March 22, 2023 11:23
@renovate renovate bot force-pushed the renovate/github.com-atc0005-go-teams-notify-v2-2.x branch from ccf0aa0 to 87fa4c0 Compare March 22, 2023 11:30
@nikoksr
Copy link
Owner

nikoksr commented Mar 22, 2023

@atc0005 thanks for reaching out! Yes, exactly, in particular the following paragraph of yours from the description of atc0005/go-teams-notify/issues/149 captures what broke testabilty of the Teams service for us:

A real challenge in the current package design is that it exposes the API interface instead of a concrete type. In order to expose the functionality added by atc0005/go-teams-notify#134 and atc0005/go-teams-notify#135, we'll need to expose the "client" (renaming teamsClient to TeamsClient) or provide a new variation of the API interface (e.g., APIv2).

The fact that teamsClient gets returned but is not exported broke our ability to mock the interface as you can see here.

I hope this helps! Cheers

@nikoksr nikoksr closed this Mar 22, 2023
@renovate
Copy link
Contributor Author

renovate bot commented Mar 22, 2023

Renovate Ignore Notification

Because you closed this PR without merging, Renovate will ignore this update (v2.7.0). You will get a PR once a newer version is released. To ignore this dependency forever, add it to the ignoreDeps array of your Renovate config.

If you accidentally closed this PR, or if you changed your mind: rename this PR to get a fresh replacement PR.

@renovate renovate bot deleted the renovate/github.com-atc0005-go-teams-notify-v2-2.x branch March 22, 2023 11:51
@atc0005
Copy link

atc0005 commented Mar 22, 2023

@nikoksr

I think I understand.

It's not that recent releases have breaking changes (including the v2.7.0 update), it's that breaking changes occurred at all.

I understand that you've already made the decision to move away from using the library, so I don't provide this information in an attempt to sway you, but I wanted to make sure that the changes applied and my notes surrounding them were not misleading.

@atc0005 thanks for reaching out! Yes, exactly, in particular the following paragraph of yours from the description of atc0005/go-teams-notify/issues/149 captures what broke testabilty of the Teams service for us:

A real challenge in the current package design is that it exposes the API interface instead of a concrete type. In order to expose the functionality added by atc0005/go-teams-notify#134 and atc0005/go-teams-notify#135, we'll need to expose the "client" (renaming teamsClient to TeamsClient) or provide a new variation of the API interface (e.g., APIv2).

Once I realized that introducing those changes to the exported API type would break any clients that relied on that API, I rolled the changes back in atc0005/go-teams-notify#151 to prevent that breakage. All of this was between official v2.6.1 and v2.7.0 releases, so thankfully no breaking changes made their way into a stable release.

I then reviewed the codebase and opted to retain things as they were to prevent breakage, but introduce new features via messagecard and adaptivecard packages. I then marked the older types/methods as deprecated to signal that they would remain, but were not the preferred way to use the library. I updated the examples to reflect this.

The fact that teamsClient gets returned but is not exported

That might be a spot I missed marking as deprecated. I'll have to review it closer to be sure.

That type is returned by deprecated code and is part of the original implementation. The TeamsClient type is exported and is returned by current code. I worked to implement internal interfaces to reflect shared functionality between the two sets of code (deprecated and current) to help ensure that no breaking changes were introduced in the future.

The paragraph you quoted was very much me "thinking out loud" as it were in an attempt to state the problem as I understood it to help frame next steps for resolving it in a backwards compatible way. I think I accomplished that and aside from the deprecation warnings generated by linters it seems to have worked.

broke our ability to mock the interface as you can see [here]

//go:generate mockery --name=teamsClient --output=. --case=underscore --inpackage
type teamsClient interface {
SendWithContext(ctx context.Context, webhookURL string, webhookMessage teams.MessageCard) error
SkipWebhookURLValidationOnSend(skip bool) teams.API
}
).

I pulled the latest from the main branch, updated to v2.7.0 of the library and ran go test ./.... Everything except for the syslog tests passed.

If any breakage remains please let me know as I consider that bug that needs to be resolved.

I do see the deprecation warnings that I noted:

$ staticcheck ./... | grep -i teams
service/msteams/mock_teams_client.go:18:99: goteamsnotify.MessageCard is deprecated: use messagecard.MessageCard instead.  (SA1019)
service/msteams/mock_teams_client.go:22:57: goteamsnotify.MessageCard is deprecated: use messagecard.MessageCard instead.  (SA1019)
service/msteams/ms_teams.go:12:73: teams.MessageCard is deprecated: use messagecard.MessageCard instead.  (SA1019)
service/msteams/ms_teams.go:17:21: teams.NewClient is deprecated: use NewTeamsClient() function instead.  (SA1019)
service/msteams/ms_teams.go:30:12: teams.NewClient is deprecated: use NewTeamsClient() function instead.  (SA1019)
service/msteams/ms_teams.go:61:13: teams.NewMessageCard is deprecated: use messagecard.NewMessageCard instead.  (SA1019)
service/msteams/ms_teams_test.go:56:13: teams.NewMessageCard is deprecated: use messagecard.NewMessageCard instead.  (SA1019)

If you were not moving away from the library I'd offer to submit a PR to correct those and also work with you to help identify and address any specific points of concern that you have with the library.

Since you are, I'll refrain from doing that and instead wish you well. I'll continue to follow project releases and hope to use it in the future.

Thank you for the kind response.

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

Successfully merging this pull request may close these issues.

None yet

4 participants