Skip to content
This repository has been archived by the owner on Jan 27, 2021. It is now read-only.

Accounts cli #69

Merged
merged 1 commit into from
Jul 27, 2020
Merged

Accounts cli #69

merged 1 commit into from
Jul 27, 2020

Conversation

IljaN
Copy link
Contributor

@IljaN IljaN commented Jul 27, 2020

First rudimentary version of ocis-accounts cli

ocis-accounts [list|delete|update|add|inspect]

Implements UpdateMask for the update request. Changed server-handler accordingly.
The commands use service-discovery to find the backend.

Demo Video: https://www.youtube.com/watch?v=tL8qWnIWDSY

  • Tests

Closes owncloud/product#115

@update-docs
Copy link

update-docs bot commented Jul 27, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@IljaN IljaN self-assigned this Jul 27, 2020
@IljaN IljaN marked this pull request as draft July 27, 2020 10:51
@IljaN IljaN requested review from butonic, kulmann, C0rby and refs July 27, 2020 10:52
@IljaN IljaN mentioned this pull request Jul 27, 2020
@IljaN
Copy link
Contributor Author

IljaN commented Jul 27, 2020

Do we want to keep the ascii-table style?

@IljaN IljaN marked this pull request as ready for review July 27, 2020 11:01
@IljaN IljaN changed the title Accounts cli Accounts cli [WIP] Jul 27, 2020
@@ -81,14 +83,6 @@ func (s Service) loadAccount(id string, a *proto.Account) (err error) {
return
}

// loggableAccount redacts the password from the account
func loggableAccount(a *proto.Account) *proto.Account {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@butonic Replaced this with https://github.com/owncloud/ocis-accounts/pull/69/files#diff-1bc5586c5f4afccf828e3161c29aa9f2R534 as this function was modifying the reference thus causing the json-store to having the "*REMOVED" string

@refs
Copy link
Member

refs commented Jul 27, 2020

@IljaN this set of commands will only work in the machine that the registry is running. I was considering tackling making the ocis cli agnostic of where the command is executed by providing minimal configuration. But as a first approach, this PR is totally valid :)

@IljaN IljaN changed the title Accounts cli [WIP] Accounts cli Jul 27, 2020
@IljaN
Copy link
Contributor Author

IljaN commented Jul 27, 2020

@IljaN this set of commands will only work in the machine that the registry is running.

Yes we decided that this is fine for a first version. It also works via etcd btw 😎, so you need to be in the service-mesh net in a distributed setup.

}

// debugLogAccount returns a debug-log event with detailed account-info, and filtered password data
func (s Service) debugLogAccount(a *proto.Account) *zerolog.Event {
Copy link
Member

Choose a reason for hiding this comment

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

for debbugging logging the account properties as fields is ok. not recommended for production because properties might collide if they are not scoped by an account object.

pkg/flagset/flagset.go Show resolved Hide resolved
ocis-accounts [list|delete|update|add|inspect]

Implements UpdateMask for the update request. Changed server-handler accordingly.
The commands use service-discovery to discover the backend.
@ownclouders
Copy link

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- pkg/service/v0/accounts.go  1
- pkg/command/list_accounts.go  2
- pkg/flagset/flagset.go  1
- pkg/command/remove_account.go  3
- pkg/command/inspect_account.go  3
- pkg/command/add_account.go  7
- pkg/command/update_account.go  5
         

Clones added
============
- pkg/flagset/flagset.go  27
- pkg/command/remove_account.go  1
- pkg/command/inspect_account.go  1
         

Clones removed
==============
+ pkg/service/v0/accounts.go  -5
         

See the complete overview on Codacy

@IljaN IljaN requested a review from butonic July 27, 2020 16:46
@IljaN
Copy link
Contributor Author

IljaN commented Jul 27, 2020

  • Added samaccountname
  • Prevent empty updates
  • Added virtual username

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Comment on lines +23 to +37
// Write value of username to the flags beneath, as preferred name
// and on-premises-sam-account-name is probably confusing for users.
if username := c.String("username"); username != "" {
if !c.IsSet("on-premises-sam-account-name") {
if err := c.Set("on-premises-sam-account-name", username); err != nil {
return err
}
}

if !c.IsSet("preferred-name") {
if err := c.Set("preferred-name", username); err != nil {
return err
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

👍

}

if c.NumFlags() == 0 {
return errors.New("missing attribute-flags for update")
Copy link
Member

Choose a reason for hiding this comment

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

👍

@butonic butonic merged commit f55894e into master Jul 27, 2020
@delete-merged-branch delete-merged-branch bot deleted the accounts-cli branch July 27, 2020 19:40
@micbar micbar mentioned this pull request Jul 29, 2020
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI for user management
4 participants