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: Rename SecretPath to SecretName #551

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

FelixTing
Copy link
Member

@FelixTing FelixTing commented May 31, 2023

fix: #550

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/go-mod-bootstrap/blob/main/.github/Contributing.md

PR Checklist

Please check if your PR fulfills the following requirements:

  • I am not introducing a breaking change (if you are, flag in conventional commit message with BREAKING CHANGE: describing the break)
  • I am not introducing a new dependency (add notes below if you are)
  • I have added unit tests for the new feature or bug fix (if not, why?)
  • I have fully tested (add details below) this the new feature or bug fix (if not, why?)
  • I have opened a PR for the related docs change (if not, why?)
    fix: Rename ExternalMqtt.SecretPath to SecretName edgex-docs#1095

Testing Instructions

New Dependency Instructions (If applicable)

@cloudxxx8 cloudxxx8 requested a review from lenny-goodell May 31, 2023 11:58
@cloudxxx8
Copy link
Member

@lenny-intel please see whether we can fix this issue in 3.0

@codecov-commenter
Copy link

Codecov Report

Merging #551 (8539625) into main (3568057) will not change coverage.
The diff coverage is 0.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##             main     #551   +/-   ##
=======================================
  Coverage   57.64%   57.64%           
=======================================
  Files          27       27           
  Lines        2236     2236           
=======================================
  Hits         1289     1289           
  Misses        865      865           
  Partials       82       82           
Impacted Files Coverage Δ
bootstrap/handlers/externalmqtt.go 0.00% <0.00%> (ø)
config/types.go 12.76% <ø> (ø)

@lenny-goodell
Copy link
Member

@lenny-intel please see whether we can fix this issue in 3.0

@cloudxxx8 , Dang it. :-( We would have to delay the release for this and IMO it isn't worth it for a minor consistency issue. It will have to wait until 4.0.

@lenny-goodell lenny-goodell marked this pull request as draft May 31, 2023 15:25
@cloudxxx8
Copy link
Member

@lenny-intel it is a real bug, not a consistency issue, so we can fix it in v3.1.

@lenny-goodell lenny-goodell added hold Intended for PRs we want to flag for ongoing review breaking-change labels Jun 1, 2023
@lenny-goodell
Copy link
Member

@lenny-intel it is a real bug, not a consistency issue, so we can fix it in v3.1.

@cloudxxx8 , How is this a real bug, rather than a name consistency issue?

It is a breaking change to 3.0.0. Should discuss in Planning next week.

@cloudxxx8
Copy link
Member

@lenny-intel it's my misunderstanding of the error log. I thought code tried to access SecretName anyway.
On the other hand it's the bug in the core-command config. We are using SecretName in core-command.
https://github.com/edgexfoundry/edgex-go/blob/1fb6775d8804dda106fccace673c66bb187c3e57/cmd/core-command/res/configuration.yaml#L31

We have to change the config or merge this PR in v3.1.

@lenny-goodell lenny-goodell added bug Something isn't working and removed hold Intended for PRs we want to flag for ongoing review breaking-change bug Something isn't working labels Jun 7, 2023
@lenny-goodell
Copy link
Member

@FelixTing , since the configuration YAML has the correct name, this is bug fix and not a breaking change as I first thought.
Please update the commit message an PR to remove specifying it as breaking change. THX!

@lenny-goodell lenny-goodell marked this pull request as ready for review June 7, 2023 21:50
Copy link
Member

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

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

LGTM once specifying breaking change is removed.

@FelixTing FelixTing changed the title refactor!: Rename SecretPath to SecretName fix: Rename SecretPath to SecretName Jun 8, 2023
@FelixTing FelixTing requested a review from lenny-goodell June 8, 2023 03:23
Copy link
Member

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

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

LGTM

@lenny-goodell lenny-goodell merged commit 2d38a8b into edgexfoundry:main Jun 8, 2023
@FelixTing FelixTing deleted the issue-550 branch June 9, 2023 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

External MQTT Rename SecretPath to SecretName
4 participants