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

OCM-4965: Keyring configuration storage #600

Merged
merged 1 commit into from
Apr 9, 2024
Merged

OCM-4965: Keyring configuration storage #600

merged 1 commit into from
Apr 9, 2024

Conversation

tylercreller
Copy link
Member

@tylercreller tylercreller commented Feb 15, 2024 •
edited
Loading

  • Added OCM_KEYRING capability to define a keyring to use for configuration storage
    • Added documentation for OCM_KEYRING usage
    • Remove configuration from keyring on logout
    • Updated Config load and save functions to account for keyrings
    • Added tests for OCM_KEYRING validation
  • Keyring tests for wincred, keychain, pass

@tylercreller tylercreller marked this pull request as draft February 16, 2024 13:54
@tylercreller tylercreller force-pushed the OCM-4965 branch 2 times, most recently from 4fbcd95 to 3689439 Compare February 19, 2024 16:18
@tylercreller tylercreller marked this pull request as ready for review February 19, 2024 16:47
tirthct
tirthct reviewed Feb 19, 2024
View reviewed changes
cmd/ocm/config/get/get.go Outdated Show resolved Hide resolved
tirthct
tirthct reviewed Feb 19, 2024
View reviewed changes
cmd/ocm/config/get/get.go Outdated Show resolved Hide resolved
@tirthct
Copy link
Contributor

tirthct commented Feb 19, 2024

Is it possible to add tests for non-existing keyrings?
Also, should we check for existing config to make sure keyrings are upserted?

@tylercreller tylercreller force-pushed the OCM-4965 branch 2 times, most recently from 67c6d03 to 9416d7d Compare February 20, 2024 20:10
@tylercreller
Copy link
Member Author

tylercreller commented Feb 20, 2024 •
edited
Loading

@tirthct

Is it possible to add tests for non-existing keyrings? Also, should we check for existing config to make sure keyrings are upserted?

Added additional tests for keychain, pass, and wincred keyrings

@tirthct
Copy link
Contributor

tirthct commented Feb 22, 2024

LGTM. The MR has some conflicts though

@tylercreller tylercreller marked this pull request as draft February 22, 2024 13:06
@tylercreller tylercreller force-pushed the OCM-4965 branch 4 times, most recently from 65bbdf4 to 44aa978 Compare March 29, 2024 18:33
@tylercreller tylercreller marked this pull request as ready for review April 2, 2024 12:51
@tirthct
Copy link
Contributor

tirthct commented Apr 2, 2024

lgtm

cristianoveiga
cristianoveiga reviewed Apr 5, 2024
View reviewed changes
pkg/config/config.go Outdated Show resolved Hide resolved
cristianoveiga
cristianoveiga reviewed Apr 5, 2024
View reviewed changes
pkg/config/config.go Outdated Show resolved Hide resolved
@tylercreller tylercreller force-pushed the OCM-4965 branch 2 times, most recently from 9252167 to 3435bfe Compare April 5, 2024 15:25
cristianoveiga
cristianoveiga approved these changes Apr 5, 2024
View reviewed changes
Copy link
Contributor

@cristianoveiga cristianoveiga left a comment

Choose a reason for hiding this comment

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

LGTM

@cristianoveiga cristianoveiga merged commit 42a6c63 into openshift-online:main Apr 9, 2024
4 checks passed
@tylercreller tylercreller mentioned this pull request Jul 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tirthct tirthct tirthct approved these changes

@cristianoveiga cristianoveiga cristianoveiga approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tylercreller @tirthct @cristianoveiga