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.

Sign up for GitHub

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

Jump to bottom

refactor: moving consul access and role interface #163

Merged
merged 1 commit into from
Oct 19, 2022
Merged

refactor: moving consul access and role interface #163

merged 1 commit into from
Oct 19, 2022

Conversation

ItsRico
Copy link
Contributor

@ItsRico ItsRico commented Oct 12, 2022 •
edited
Loading

Moving configureConsulAccess and createRole to go-mod-secrets to consolidate consul API.

Closes: #3227

Signed-off-by: Rico Chavez-Lopez rchavezlopez@ucdavis.edu

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-secrets/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)
  • [ x] I am not introducing a new dependency (add notes below if you are)
  • [ x] I have added unit tests for the new feature or bug fix (if not, why?)
  • [x ] 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?)

Testing Instructions

Please refer to edgexfoundry/edgex-go#4193

New Dependency Instructions (If applicable)

@ItsRico ItsRico closed this Oct 12, 2022
@ItsRico ItsRico reopened this Oct 12, 2022
@bnevis-i bnevis-i added the hold Intended for PRs we want to flag for ongoing review label Oct 12, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #163 (7350728) into main (7064924) will decrease coverage by 0.12%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main     #163      +/-   ##
==========================================
- Coverage   77.55%   77.42%   -0.13%     
==========================================
  Files          17       18       +1     
  Lines         940      988      +48     
==========================================
+ Hits          729      765      +36     
- Misses        150      162      +12     
  Partials       61       61
Impacted Files Coverage Δ
pkg/types/helpers.go 0.00% <0.00%> (ø)
internal/pkg/vault/management.go 95.67% <100.00%> (+1.23%) ⬆️

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

bnevis-i
bnevis-i previously approved these changes Oct 12, 2022
View reviewed changes
Copy link
Collaborator

@bnevis-i bnevis-i left a comment

Choose a reason for hiding this comment

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

LGTM. Merging hold until have corrected edgex-go PR to test against.

@ItsRico ItsRico mentioned this pull request Oct 12, 2022
Merged
1 task
jim-wang-intel
jim-wang-intel previously approved these changes Oct 12, 2022
View reviewed changes
@ItsRico ItsRico dismissed stale reviews from jim-wang-intel and bnevis-i via 64f3ba8 October 18, 2022 08:49
@ItsRico
Copy link
Contributor Author

ItsRico commented Oct 18, 2022

  • changed helpers.go to registryroles.go
  • moved registry roles & policy structs to registryroles
  • added copyright to top of registryroles.go
  • Changed casing of CreateRole and ConfigureConsulAcess' comments

lenny-goodell
lenny-goodell requested changes Oct 18, 2022
View reviewed changes
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.

Code looks good, but need to change all usages of registry to be Consul. I have flagged a bunch of them, but there are more.

internal/pkg/vault/management.go Outdated Show resolved Hide resolved
internal/pkg/vault/management.go Outdated Show resolved Hide resolved
internal/pkg/vault/management.go Outdated Show resolved Hide resolved
pkg/types/registryroles.go Outdated Show resolved Hide resolved
pkg/types/registryroles.go Outdated Show resolved Hide resolved
pkg/types/registryroles.go Outdated Show resolved Hide resolved
pkg/types/registryroles.go Outdated Show resolved Hide resolved
pkg/types/registryroles.go Outdated Show resolved Hide resolved
internal/pkg/vault/management.go Outdated Show resolved Hide resolved
internal/pkg/vault/management.go Outdated Show resolved Hide resolved
@ItsRico
Copy link
Contributor Author

ItsRico commented Oct 19, 2022

Updated all references of registry to consul

lenny-goodell
lenny-goodell approved these changes Oct 19, 2022
View reviewed changes
Copy link
Member

@lenny-goodell lenny-goodell left a comment •
edited
Loading

Choose a reason for hiding this comment

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

One minor tweak needed.

internal/pkg/vault/management.go Outdated Show resolved Hide resolved
@lenny-goodell lenny-goodell self-requested a review October 19, 2022 01:46
lenny-goodell
lenny-goodell requested changes Oct 19, 2022
View reviewed changes
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.

One minor tweak needed

@lenny-goodell lenny-goodell marked this pull request as ready for review October 19, 2022 01:49
@ItsRico
refactor: moving consul access and role interface
29a3a64 
Moving configureConsulAccess and createRole to go-mod-secrets to
consolidate consul API.

Closes: #3227

Signed-off-by: Rico Chavez-Lopez <rchavezlopez@ucdavis.edu>
lenny-goodell
lenny-goodell approved these changes Oct 19, 2022
View reviewed changes
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

@bnevis-i bnevis-i merged commit 7e745c8 into edgexfoundry:main Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnevis-i bnevis-i bnevis-i approved these changes

@lenny-goodell lenny-goodell lenny-goodell approved these changes

@jim-wang-intel jim-wang-intel jim-wang-intel left review comments

Assignees
No one assigned
Labels
hold Intended for PRs we want to flag for ongoing review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ItsRico @codecov-commenter @bnevis-i @jim-wang-intel @lenny-goodell