govc: Add -file flag to cluster.module.rm to delete a list of cluster modules read from a file

[chrischdi]

Description

This PR adds a -file flag to the govc cluster.module.rm command.

Deleting lots of cluster modules takes a long time.

E.g. deleting 10k cluster modules using bash and the current implementation would take ~5 mins. Example command:

while read -r ID; do govc cluster.module.rm $ID ; done < modules.txt

With this PR the following would do the same and only take ~30s:

govc cluster.module.rm - < modules.txt

Closes: #(issue-number)

Type of change

Please mark options that are relevant:

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to
  not work as expected)
• This change requires a documentation update
• Build related change

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide
instructions so we can reproduce. If applicable, please also list any relevant
details for your test configuration.

• Created 10k cluster modules and deleted using the new flag
  • extracted all existing modules via govc cluster.module.ls | awk '{print $2}' > modules.txt
  • ran govc cluster.module.rm -file modules.txt

Checklist:

• My code follows the CONTRIBUTION guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged

[chrischdi]

Many thanks @bryanv for the review. I addressed the comments. I should next time not rush a PR before calling it a day 🤦

[hickeng]

Unless the "list in a file" is a pattern used elsewhere, consider instead the "read from stdin when - is specified" pattern.

That's a standard unix idiom and allows for use in scripting pipelines.
When used as an isolated command either a < redirection or a HERE document can be used to transfer a file/inline list to stdin.

[hickeng]

Consider whether you want another flag to continue processing modules on error, or to ignore "unknown module" errors.

Noting because if you end up with a failure somewhere in the 10k, you cannot simply resubmit the file but have to edit it to remove all modules that have already been deleted.

Not blocking, but a nice refinement. If needing to test this the simulator has module support, see this line

[dougm]

Unless the "list in a file" is a pattern used elsewhere, consider instead the "read from stdin when - is specified" pattern.

We have a few cases where - == stdin, such as {guest,datastore}.upload commands. But we don't have a pattern where a -file flag is used to specify N arguments (such as IDs in this case)

We already have inconsistency where some commands take just the argument needed for a single API call (such as cluster.module.rm currently), but others take N arguments and call the method N within its own loop (such as vm.destroy).
So I'd prefer not to add a special case -file flag to this command, but would be ok with
govc cluster.module.rm - reading a list of IDs from stdin.

We could later add helpers to apply this pattern and others such as parallel API calls, etc., to a broader set of commands.

Note that we do also have a way to hide commands and flags by default:

govmomi/govc/cli/register.go

Lines 25 to 32 in ba843e5

	// hideUnreleased allows commands to be compiled into the govc binary without being registered by default.
	// Unreleased commands are omitted from 'govc -h' help text and the generated govc/USAGE.md
	// Setting the env var GOVC_SHOW_UNRELEASED=true enables any commands registered as unreleased.
	var hideUnreleased = os.Getenv("GOVC_SHOW_UNRELEASED") != "true"
	func ShowUnreleased() bool {
	return !hideUnreleased
	}

Used with a flag here for example:

govmomi/govc/library/deploy.go

Lines 66 to 68 in ba843e5

	if cli.ShowUnreleased() {
	f.StringVar(&cmd.config, "config", "", "VM config spec")
	}

[chrischdi]

Wondering if we should instead export a function at the library to match the error via a typecast or similar.

[dougm]

Meant to add, we have an issue open for this: #2521

[chrischdi]

Thanks for the great response. Using stdin also makes the implementation more clean and fulfils the same functionality 👍

I also added bats tests to cover that functionality :-)

[chrischdi]

@dougm and @hickeng kindly asking for giving this another look :-)

[dougm]

Thanks @chrischdi , lgtm

[dougm]

Meant to add, we have an issue open for this: #2521