8000 feat: implement patch identity command by pyneda · Pull Request #3055 · ory/kratos · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: implement patch identity command #3055

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
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

feat: implement patch identity command #3055

wants to merge 5 commits into from
+194 −0

Conversation

pyneda
Copy link
@pyneda pyneda commented Jan 28, 2023

This PR implements the missing patch identity command

Related issue(s)

N/A

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

Once merged I could also implement it into the ory cli

@CLAassistant
Copy link
CLAassistant commented Jan 28, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link
codecov bot commented Jan 28, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 82.43243% with 13 lines in your changes missing coverage. Please review.

Project coverage is 77.30%. Comparing base (44556a4) to head (341a196).
Report is 1036 commits behind head on master.

Files with missing lines Patch % Lines
cmd/identities/patch.go 82.19% 8 Missing and 5 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3055      +/-   ##
==========================================
+ Coverage   77.28%   77.30%   +0.01%     
==========================================
  Files         313      314       +1     
  Lines       19552    19626      +74     
==========================================
+ Hits        15110    15171      +61     
- Misses       3274     3282       +8     
- Partials     1168     1173       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

aeneasr
aeneasr requested changes Jan 30, 2023
View reviewed changes
Copy link
Member
@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Very cool! I have a suggestion to improve the CLI flag and streamline it with other patch comamnds :)

cmd/identities/patch.go
Comment on lines +60 to +63
from := flagx.MustGetString(cmd, "from")
path := flagx.MustGetString(cmd, "path")
value := flagx.MustGetString(cmd, "value")
if value != "" && op != "add" && op != "replace" {
Copy link
Member

Choose a reason for hiding this comment

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

Check out how we do this in the Ory CLI right now:

https://github.com/ory/cli/blob/6edb990eb6ba8902f6e55feb9b646db6f695e195/cmd/cloudx/project/patch_identity_config.go#L59-L61

This allows us to have multiple operations in one CLI call :) WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aeneasr aeneasr aeneasr requested changes

@zepatrik zepatrik Awaiting requested review from zepatrik

Requested changes must be addressed to merge this pull request.

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
@pyneda @CLAassistant @aeneasr
0