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

Expose BatchEntryDelete On CLI #4371

Merged

azdagron merged 6 commits into spiffe:main from JU4N98:expose-batch-delete-entry-on-cli

Aug 29, 2023

Contributor

JU4N98 commented Jul 27, 2023 •

edited

Loading

Affected functionality
This PR affects the entry delete on cli.

Description of change
This PR allows:

The use of entryID flag multiple times (like selector flag of entry create).
The use of the new flag data, which specifies the path to a JSON with the entryIDs to be deleted or just "-" in case that the JSON should be taken from stdin. Follows an example of this JSON:

{
    Ids : ["entry-0", "entry-1"]
}

In both cases, the output is similar to entry create, first the succeeded entry deletions are enumerated and then the failed ones.

Which issue this PR fixes
This PR fixes the issue #4248.

JU4N98 added 2 commits

July 27, 2023 13:59


          Allows the use of multiple entryID or a JSON as input of delete.

70f6b32

Signed-off-by: JU4N98 <juanpablocabana2@gmail.com>


          Fixes misspelling and other lint errors.

774aee9

Signed-off-by: JU4N98 <juanpablocabana2@gmail.com>

JU4N98 requested review from evan2645, amartinezfayo, azdagron, MarcosDY and rturner3 as code owners

July 27, 2023 18:08

amartinezfayo assigned evan2645

evan2645 reviewed

View reviewed changes

Member

evan2645 left a comment

Thank you very much for the contribution @JU4N98! And sorry for the delay on my part

Left a handful of comments, mostly around UX. It leads me to wonder about the output we print ... if folks are providing programmatic input, should we be providing programatic output? What is the path to recovering from a batch delete call where half the requests failed? just thinking out loud ...

cmd/spire-server/cli/entry/delete.go Outdated

-              	entryID string
-              	env     *commoncli.Env
-              	printer cliprinter.Printer
+              	entriesIDs StringsFlag

Member

evan2645 Aug 15, 2023

nit, this should probably be

Suggested change

      
            	entriesIDs StringsFlag
          
            	entryIDs StringsFlag

cmd/spire-server/cli/entry/delete.go Outdated

@@ @@ -40,16 +43,54 @@ func (*deleteCommand) Synopsis() string { @@
               }
               func (c *deleteCommand) AppendFlags(f *flag.FlagSet) {
-              	f.StringVar(&c.entryID, "entryID", "", "The Registration Entry ID of the record to delete")
+              	f.Var(&c.entriesIDs, "entryID", "The Registration Entry ID of the record to delete. Can be used more than once")
+              	f.StringVar(&c.path, "data", "", "Path to a file containing deletion JSON (optional). If set to '-', read the JSON from stdin.")

Member

evan2645 Aug 15, 2023

Can we change this flag name to file instead? I think it's a bit clearer that way

Member

evan2645 Aug 15, 2023

I think we should mention something about batch deletion here ... maybe like:

Suggested change

      
            	f.StringVar(&c.path, "data", "", "Path to a file containing deletion JSON (optional). If set to '-', read the JSON from stdin.")
          
            	f.StringVar(&c.path, "data", "", "Path to a file containing a JSON structure for batch deletion (optional). If set to '-', read from stdin.")

Contributor Author

JU4N98 Aug 16, 2023

Sure, i think that file is a better description than data.

cmd/spire-server/cli/entry/delete.go Outdated

@@ @@ -40,16 +43,54 @@ func (*deleteCommand) Synopsis() string { @@
               }
               func (c *deleteCommand) AppendFlags(f *flag.FlagSet) {
-              	f.StringVar(&c.entryID, "entryID", "", "The Registration Entry ID of the record to delete")
+              	f.Var(&c.entriesIDs, "entryID", "The Registration Entry ID of the record to delete. Can be used more than once")

Member

evan2645 Aug 15, 2023

I don't think we should allow multiple entryID flags to be passed. It definitely feels like the natural thing to do, but there are other batch endpoints that we'll need to support on CLI (like entry create) that can't be supported in this way ... so to keep things consistent, I feel we should support file-based batch operations only

Contributor Author

JU4N98 Aug 16, 2023

Yes, you are right, i didn't notice that entry create doesn't support batch create without a file. I'm going to fix that and keep the file flag.

cmd/spire-server/cli/entry/delete.go

+              	}
+              	for _, result := range failed {
+              		env.ErrPrintf("Failed to delete entry with ID %s (code: %s, msg: %q)\n",

Member

evan2645 Aug 15, 2023

We should probably print out a newline or two between the success and error lines

cmd/spire-server/cli/entry/delete.go

+              			codes.Code(result.Status.Code),
+              			result.Status.Message)
+              	}

Member

evan2645 Aug 15, 2023

I think we should print a newline here and another message summarizing, e.g. "Deleted X entries successfully" or "Deleted X entries successfully, but failed to delete Y entries"

cmd/spire-server/cli/entry/delete.go

+              	}
+              	if len(failed) > 0 {
+              		return errors.New("failed to delete one or more entries")

Member

evan2645 Aug 15, 2023

The CLI command should only error if the call as a whole has errored

Contributor Author

JU4N98 Aug 16, 2023

I followed create.go as a guide:

spire/cmd/spire-server/cli/entry/create.go

Line 284 in 5aa2122

if len(failed) > 0 {

Maybe I misunderstood the code of entry create, but I think that we are doing the same there.

Thanks for the comments!

Member

evan2645 Aug 22, 2023

Ah ok ... consistency is best 😊 thanks for pointing that out


          Adds suggestions.

84cab98

Signed-off-by: JU4N98 <juanpablocabana2@gmail.com>

evan2645 previously approved these changes

View reviewed changes

Member

evan2645 left a comment

Thank you @JU4N98 !!

Member

evan2645 commented Aug 24, 2023

Hey @JU4N98, looks like the Windows unit tests need to be fixed up following changes to help text output: https://github.com/spiffe/spire/actions/runs/5907969226/job/16186854897?pr=4371

Mind patching it so we can get this merged in for 1.8.0?


          Fixes windows unit test.

e447345

Signed-off-by: JU4N98 <juanpablocabana2@gmail.com>

JU4N98 dismissed evan2645’s stale review via

e447345

August 24, 2023 19:33


          Fixes windows unit test.

8d9968c

Signed-off-by: JU4N98 <juanpablocabana2@gmail.com>

Contributor Author

JU4N98 commented Aug 25, 2023 •

edited

Loading

Hey @JU4N98, looks like the Windows unit tests need to be fixed up following changes to help text output: https://github.com/spiffe/spire/actions/runs/5907969226/job/16186854897?pr=4371

Mind patching it so we can get this merged in for 1.8.0?

hey @evan2645 windows unit test should be fixed right now

evan2645 approved these changes

View reviewed changes


          Merge branch 'main' into expose-batch-delete-entry-on-cli

7e1533e

azdagron merged commit 7a2dcfb into spiffe:main

31 checks passed

azdagron added this to the 1.8.0 milestone

rturner3 mentioned this pull request

v1.8.0 CHANGELOG #4497

Merged

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

Reviewers

evan2645 evan2645 approved these changes

amartinezfayo Awaiting requested review from amartinezfayo amartinezfayo is a code owner

azdagron Awaiting requested review from azdagron azdagron is a code owner

MarcosDY Awaiting requested review from MarcosDY MarcosDY is a code owner

rturner3 Awaiting requested review from rturner3 rturner3 is a code owner

Labels

None yet