New Command: Mixer bundle edit #155

kevincwells · 2018-02-08T04:55:44Z

Adds new command, 'mixer bundle edit', that allows a user to edit local
and upstream bundle definition files. This command will locate the
bundle (looking first in local-bundles, then in upstream-bundles), and launch
an editor to edit it. If the bundle is only found upstream, the bundle file will
first be copied to your local-bundles directory for editing. If the bundle is
not found anywhere, a blank template is created with the correct name. When the
editor closes, the bundle file is then parsed for validity.

The editor is configured via environment variables. VISUAL takes precedence to
EDITOR. If neither are set, the tool defaults to nano. If nano is not installed,
the tool will skip editing, and act as if '--suppress-editor' had been passed.

Passing '--suppress-editor' will suppress launching the editor, and will thus
only copy the bundle file to local-bundles (if it is only found upstream), or
create the blank template (if it was not found anywhere). This can be useful if
you want to add a bundle to local-bundles, but wish to edit it at a later time.

Passing '--add' will also add the bundle(s) to your mix. Please note that
bundles are added after all bunles are edited, and thus will not be added if any
errors are encountered earlier on.

Signed-off-by: Kevin C. Wells kevin.c.wells@intel.com

kevincwells · 2018-02-08T05:01:13Z

helpers/helpers.go

@@ -294,6 +294,7 @@ func RunCommand(cmdname string, args ...string) error {
 	cmd := exec.Command(cmdname, args...)
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
+	cmd.Stdin = os.Stdin


This is added to facilitate launching applications that read input (i.e., the editors). I considered adding the same thing to the RunCommandOutput function below, but didn't know how people would want it. Perhaps a separate RunCommandInput function that takes a bytes.Buffer as input? We can add that if we have a need.

I made sure the helpers_tests still ran as expected after adding this line.

This is fine until you have more than one go routine calling RunCommand, at which point you have a fight over which program gets the terminal input.

I think you should create a new routine RunCommandInput that takes an io.Reader and has 98% of the body of this function, then make RunCommand a one liner, to call RunCommandInput with nil for this parameter.

Great idea. I'll redo this.

icarus-sparry

I would find this pretty annoying to use, I think you should allow users to choose their own editor rather than being given a list. I think if something fails to parse you should offer immediately to allow the user to fix the error rather than tell them to re-edit. I think it is worth suggesting that you keep a copy of the file so the user can abandon the edit. Imaging the user deletes the entire contents of the file, telling him to fix it is not very helpful.

icarus-sparry · 2018-02-08T07:39:36Z

builder/builder.go

+	if editor != "" {
+		b.BundleEditor = editor
+	}
+	editorCmd, exists := validEditors[b.BundleEditor]


I think this is very misguided. For example xemacs and gvim are not in the list, which we do ship. Trust your users!

In the past for automation I have written scripts to edit files.

The thing I would expect is

If I pass an editor to use, then use that.

(optional) if the environment variable BUNDLE_EDITOR is set, use the value of that

If the environment variable VISUAL is set, use the value of that.

If the environment variable EDITOR is set, use the value of that.

Use 'vi' (possibly nano as it is in sysadmin-basic)

Personally I would be inclined to not have an option to pass in the editor via a flag at all.

This is actually exactly the original way I had this implemented -- if the user sets (or passes in) the path to the editor, use that, else try to look up $VISUAL, then $EDITOR, then fall back to nano.

The reason I went away from this is that, while this flexibility is awesome, basic security policy says to explicitly not "trust your users!". Whatever you configure here is going to just be passed off to an exec, effectively. Unlike git, which lets you configure whatever editor you want for merge conflicts, mixer is run via sudo. Yes, these commands require no elevated privileges, but when so many of the others do, users will absolutely run them with sudo either out of a) laziness, b) misunderstanding, or c) because the files they're editing are owned by root because of some other operation.

As far as the list not including xemacs or gvim, I just pulled the list from the packages included in editors bundle -- if those packages include other variants I left out, I'm happy to add them.

Just to be clear (as I hope is evidenced by the fact that I originally implemented it the way you suggested), I also don't love the idea of limiting this to a small list of editors. I am absolutely open to ways to open this up that don't introduce mixer executing random binaries as root. :)

I do understand your concern, but I think you are fixing the problem in the wrong place. We have other of those customizations of what program to call (BuildScript, --packager), that I think is unfeasible to whitelist all of them.

Also, we really don't want the user to create those files with root permissions (if he's keeping track of the repository in git for example).

I think the real way to deal with this is to contain where we really need sudo, and possibly call sudo ourselves when we really need (which also might be controversial, but I think is a fight on the right battle).

For this functionality (which I think is very good btw!), I'd say either:

Just do what you did before or Icarus is suggesting

Avoid editing for now and keep in some form the "copy this manifest from upstream" functionality

Figure out a way to safely "drop privileges" by setuid yourself back to the original user when running this command (kind of the reverse of sudo). I don't think Go handles that very well yet, though. (See their issue 1435).

Personally I would be inclined to not have an option to pass in the editor via a flag at all.

I agree with that one, we can defer the flag until a real need surfaces. The EDITOR and [name]_EDITOR are common enough in the command line.

The problem is that even if you restrict the user to only running 'vi' you can fire up vi. You then type ":sh" and enter, and you have a shell running as root! At that point you can run anything you like as root.

sudo is the wrong tool for increasing privileged status. We ought to see exactly what wants 'root', and see if we can eliminate the need as a medium term goal.

We have other of those customizations of what program to call (BuildScript, --packager), that I think is unfeasible to whitelist all of them

BuildScript is hard-coded (and not read in from the config), and for --packager, you really are running exec on whatever the user passes in? I didn't review that part of the new BCB, but I would have definitely flagged that if I had.

Also, we really don't want the user to create those files with root permissions (if he's keeping track of the repository in git for example).

Agreed.

I think the real way to deal with this is to contain where we really need sudo

👍

Figure out a way to safely "drop privileges" by setuid yourself back to the original user when running this command (kind of the reverse of sudo). I don't think Go handles that very well yet, though. (See their issue 1435).

I've done this in other languages before. I'll look into if this is feasible.

We ought to see exactly what wants 'root', and see if we can eliminate the need as a medium term goal.

@icarus-sparry created #158 with some more data on this. Want to tackle that problem? :-)

BuildScript is hard-coded (and not read in from the config),

That was surprising. I guess it was made that way to be configurable at some point. That should be a constant.

and for --packager, you really are running exec on whatever the user passes in? I didn't review that part of the new BCB, but I would have definitely flagged that if I had.

Yep, to let the user pick some other dnf (either the system or a compiled one for testing). In that case, we could've retrofit it to be a flag that just says "use dnf" and use whatever is in PATH.

I guess the two examples were not that strong but I stand by the other arguments I gave :-)

icarus-sparry · 2018-02-08T07:49:54Z

helpers/helpers.go

@@ -294,6 +294,7 @@ func RunCommand(cmdname string, args ...string) error {
 	cmd := exec.Command(cmdname, args...)
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
+	cmd.Stdin = os.Stdin


This is fine until you have more than one go routine calling RunCommand, at which point you have a fight over which program gets the terminal input.

I think you should create a new routine RunCommandInput that takes an io.Reader and has 98% of the body of this function, then make RunCommand a one liner, to call RunCommandInput with nil for this parameter.

icarus-sparry · 2018-02-08T07:51:04Z

builder/builder.go

+	}
+
+	if add {
+		if err := b.AddBundles(bundles, false, false, false); err != nil {


Can just return b.AddBundles(.......)

Nice, thanks.

icarus-sparry · 2018-02-08T07:52:09Z

builder/builder.go

+			path = localPath
+		}
+
+		if !noEdit {


Maybe

if noEdit { continue }

I like this, because it removes the double-negative (and some indentation).

icarus-sparry · 2018-02-08T07:54:57Z

builder/builder.go

+	"vim":   "/usr/bin/vim",
+}
+
+//EditBundles prints out a bundle list in either a flat list or tree view


Is this comment correct?

Some documentation here on the expected use of noEdit==false, and add==true/false would probably help, or just a pointer to the viper help stuff.

Oh, whoa, nope. Copypasta error.

icarus-sparry · 2018-02-08T07:59:17Z

builder/builder.go

+			}
+
+			if _, err := parseBundleFile(path); err != nil {
+				return errors.Wrapf(err, "Failed to parse bundle %s. Please edit again", bundle)


Should this offer something more like a choice Revert/Reedit/Continue (with Revert undoing any edits, Reedit looping to reinvoke the editor and Continue to move to the next bundle) rather than erroring out here?

icarus-sparry · 2018-02-08T08:01:23Z

mixer/cmd/bundles.go

+closes, the bundle file is then parsed for validity.
+
+The editor is configured in builder.conf via the BUNDLE_EDITOR field, or by flag
+override. Valid options include: nano, vim, emacs, joe, and ctags. The default


ctags is a valid editor?

Haha, is it not? Basic ignorance issue here. As mentioned above, I pulled the list from what packages are installed by editors, but I obviously should have done a basic google. Ctags is a package used by one of the actual editors?

I'll pull it.

ctags produces a 'tags' database so you can find definitions and uses of variables in C projects.

icarus-sparry · 2018-02-08T08:07:58Z

builder/builder.go

+
+		if !noEdit {
+			if err := helpers.RunCommand(editorCmd, path); err != nil {
+				return errors.Wrapf(err, "Failed to edit bundle: %s", bundle)


This is high risk. Some editors return a non-zero exit code if some commands fail, which might be as simple as a search failing.

That's a great point. In theory, I don't really care what the editor returned, I just care if I can read/parse it afterward. I should probably just ignore the error here, and let the parsing trigger the error.

icarus-sparry · 2018-02-08T08:14:22Z

mixer/cmd/bundles.go

+
+Passing '--no-edit' will suppress launching the editor, and will thus only copy
+the bundle file to local-bundles if it is only found upstream. This can be
+useful if you want to add a bundle to local-bundles, but wish to edit it at a


Is this a real use case? Would it be better as a different command?

Once you allow someone to choose their own editor, they can always use /bin/true to get the same effect.

Honestly, this is something people asked for. I don't remember exactly who or when, but when I proposed this feature back in December, this is the feedback I got -- that people would want to be able to move the file (unchanged) to local-bundles, and edit it at their leisure.

Also, while yes -- if you could pick any editor you want, you could use /bin/true -- expecting users to know or do that is the opposite of user-friendly.

I think that is an important part of the feature, because that is a boring computer task that mixer can do for us: hey just get me the desktop and desktop-apps from the upstream. I've almost suggested something like mixer bundle copy desktop desktop-apps (or copy-upstream, or import, or whatever) as a standalone thing (so you don't have to deal with the "edit but no edit").

@cmarcelo I had considered at one point making that a stand-alone mixer bundle import command, or even a flag off add (mixer bundle add --local <bundle>).

The reason I don't want that is that, while people might very well find it useful to pull things into local-bundles and edit at some later time, I want them to still be aware that doing this overrides the upstream version. If you're not actually planning to edit the bundle, having a copy in local-bundles is almost certainly just going to cause confusion later (either for you, or for the tool when it tries to migrate to new upstream version). So keeping it a part of the mixer bundle edit command reminds people why they're moving things into local-bundles: to edit them.

On that note, one (potentially valid, but dangerous) use-case for copying into local and not editing is exactly that -- persisting a bundle past a deletion in a future upstream version. What I mean is, if you add bundle_a to local-bundles, and in a future version of upstream bundle_a gets removed entirely, it now becomes equivalent to a "user-created" bundle -- it is no longer masking anything upstream. That very well might be a desirable thing for someone, but for most people that will likely just cause confusion.

icarus-sparry · 2018-02-08T08:16:14Z

mixer/cmd/bundles.go

+later time.
+
+Passing '--add' will also add the bundle(s) to your mix. Please note that
+bundles are added after all bunles are edited, and thus will not be added if any


spelling nitpick. "all bunles are"

cmarcelo

Requested change: no list of valid editors.

cmarcelo · 2018-02-08T18:58:24Z

mixer/cmd/bundles.go

+override. Valid options include: nano, vim, emacs, joe, and ctags. The default
+value is nano.
+
+Passing '--no-edit' will suppress launching the editor, and will thus only copy


mixer bundle edit --no-edit sounds strange, maybe mixer bundle edit --copy-only?

Hahah, I had that same thought last night. I couldn't come up with a better name (it was late and I was hungry), but that works pretty well.

cmarcelo · 2018-02-08T19:03:53Z

Besides the nits and the list of valid editors, this looks good to me.

kevincwells · 2018-02-09T03:10:40Z

Pushed a new version that addresses some feedback (and rebased on top of #137). Still need to swap out the validEditors and, ideally, address @icarus-sparry's usability complaints (which I agree with)

kevincwells · 2018-02-10T00:57:14Z

I just pushed an update that I believe addresses @icarus-sparry and @cmarcelo's issues.

The editor is now read from VISUAL, then EDITOR, then defaults to nano. If nano cannot be found, it skips editing and acts as if --copy-only was passed.

If parsing fails, the user is told why and prompted to choose between re-editing as-is, reverting and editing fresh, or skipping. If they skip, both the edited and a backed-up <bundle>.orig file are left behind, with output to the user telling them so.

kevincwells · 2018-02-10T01:05:41Z

FYI - mixer bundle list does not filter out invalid bundles when it runs, so if you run mixer bundle list --local while you still have a bundle.orig file laying around, it'll error out saying that is an invalid bundle name. Frankly, I think this is probably a good thing, so people don't forget that they're in a broken state.

If people think this is bad, I would suggest a future patch (not part of this) that either a) skips files with ".orig" at the end, or b) does the validBundleNameRegex.MatchString currently in parseBundleFile for something more thorough, though I do worry about skipping over "actual" misnamed bundles.

icarus-sparry

Much better thank you. I think we need a clearer prompt message.

icarus-sparry · 2018-02-10T01:06:02Z

builder/builder.go

+			if err = os.Remove(backup); err != nil {
+				return errors.Wrapf(err, "Error cleaning up backup for bundle '%s'", bundle)
+			}
+			break editLoop


Since all that 'break editloop' does is 'return nil', in this case you could just return nil. However leave it as it is, as the structure is nice.

icarus-sparry · 2018-02-10T01:17:05Z

builder/builder.go

+			text = strings.ToLower(text)
+			text = strings.TrimSpace(text)
+			switch {
+			case text == "c" || text == "continue":


When I suggested reedit/continue/restore I was rather thinking of editing more than one bundle.

So I would expect 'continue' to leave the current bundle and go on to the next, 'reedit' to rerun the editor on the current file, and reset to run the editor on a copy of the original file.

So your 'continue' is my 'reedit', your 'skip' is my 'continue' and your 'revert' is my 'restore'.

Let me first say there is nothing wrong with your names, but then say "More explanation is probably needed in the prompt text' so it is clear what 'continue' means.

icarus-sparry

LGTM

kevincwells · 2018-02-12T19:58:09Z

Pushed a minor update that adds a --git flag to keep it consistent with the other mixer bundle ____ commands existing or forthcoming, and simplifies the output wording when parsing errors are encountered.

@cmarcelo I think you were requesting the same change as Icarus; can you please check if this meets your needs too?

Adds new command, 'mixer bundle edit', that allows a user to edit local and upstream bundle definition files. This command will locate the bundle (looking first in local-bundles, then in upstream-bundles), and launch an editor to edit it. If the bundle is only found upstream, the bundle file will first be copied to your local-bundles directory for editing. When the editor closes, the bundle file is then parsed for validity. The editor is configured via environment variables. VISUAL takes precedence to EDITOR. If neither are set, the tool defaults to nano. If nano is not installed, the tool will skip editing, and act as if '--copy-only' had been passed. Passing '--copy-only' will suppress launching the editor, and will thus only copy the bundle file to local-bundles if it is only found upstream. This can be useful if you want to add a bundle to local-bundles, but wish to edit it at a later time. Passing '--add' will also add the bundle(s) to your mix. Please note that bundles are added after all bunles are edited, and thus will not be added if any errors are encountered earlier on. Signed-off-by: Kevin C. Wells <kevin.c.wells@intel.com>

cmarcelo

Looks OK but let's incorporate the new bundle functionality ;-)

kevincwells · 2018-02-13T02:04:43Z

@cmarcelo is right, and the functionality I was putting in #171 belongs here. Gets rid of a lot of similar code, too!

This command will work exactly as it does now, except if they're editing a bundle that doesn't exist upstream, it'll "create" a new bundle for them and have them edit the template.

cmarcelo · 2018-02-13T02:06:12Z

builder/builder.go

+			}
+			text = strings.ToLower(text)
+			text = strings.TrimSpace(text)
+			switch {


You can switch on strings in Go, so switch text { then use case "e", "edit": and so on.

Huh. I knew you could switch on strings, but I'd never seen the case "e", "edit": syntax before. I definitely prefer that.

Add 'create new bundle' functionality to 'mixer bundle edit'. Now, if a bundle is not found in local-bundles or upstream-bundles, it is treated as a new bundle, and a blank bundle definition file template is created for the user to edit. Also minor syntax optimization on editing switch.

kevincwells · 2018-02-13T02:58:37Z

Pushed change @cmarcelo suggested. It's pushed as a new commit, so people can review just that one, without worrying about reviewing the entire mixer bundle edit command again.

cmarcelo · 2018-02-13T02:59:33Z

LGTM.

kevincwells commented Feb 8, 2018

View reviewed changes

icarus-sparry suggested changes Feb 8, 2018

View reviewed changes

cmarcelo suggested changes Feb 8, 2018

View reviewed changes

cmarcelo reviewed Feb 8, 2018

View reviewed changes

cmarcelo mentioned this pull request Feb 8, 2018

Enumerate then contain/eliminate the places we need root #158

Closed

kevincwells force-pushed the mixer-bundle-edit branch from 210d587 to c1ed9f9 Compare February 9, 2018 03:09

kevincwells force-pushed the mixer-bundle-edit branch from c1ed9f9 to 4c7b1d5 Compare February 10, 2018 00:52

icarus-sparry reviewed Feb 10, 2018

View reviewed changes

kevincwells force-pushed the mixer-bundle-edit branch 2 times, most recently from 12795c2 to f624273 Compare February 10, 2018 01:36

icarus-sparry reviewed Feb 10, 2018

View reviewed changes

icarus-sparry approved these changes Feb 10, 2018

View reviewed changes

kevincwells force-pushed the mixer-bundle-edit branch 2 times, most recently from 9e8322b to ed17398 Compare February 12, 2018 19:57

kevincwells mentioned this pull request Feb 12, 2018

New Command: Mixer bundle create #171

Closed

kevincwells force-pushed the mixer-bundle-edit branch from ed17398 to 28833bf Compare February 13, 2018 01:28

kevincwells changed the title ~~New Command: Mixer bundle edit~~ (Temporary DO NOT MERGE): New Command: Mixer bundle edit Feb 13, 2018

cmarcelo reviewed Feb 13, 2018

View reviewed changes

cmarcelo approved these changes Feb 13, 2018

View reviewed changes

cmarcelo reviewed Feb 13, 2018

View reviewed changes

kevincwells force-pushed the mixer-bundle-edit branch from b20361a to 4157b06 Compare February 13, 2018 02:55

kevincwells changed the title ~~(Temporary DO NOT MERGE): New Command: Mixer bundle edit~~ New Command: Mixer bundle edit Feb 13, 2018

tmarcu merged commit 502f6c2 into clearlinux:master Feb 13, 2018

kevincwells deleted the mixer-bundle-edit branch February 20, 2018 20:36

New Command: Mixer bundle edit #155

New Command: Mixer bundle edit #155

Conversation

kevincwells commented Feb 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icarus-sparry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevincwells Feb 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmarcelo left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmarcelo commented Feb 8, 2018

kevincwells commented Feb 9, 2018 • edited

kevincwells commented Feb 10, 2018

kevincwells commented Feb 10, 2018 • edited

icarus-sparry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icarus-sparry left a comment

Choose a reason for hiding this comment

kevincwells commented Feb 12, 2018 • edited

cmarcelo left a comment

Choose a reason for hiding this comment

kevincwells commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevincwells commented Feb 13, 2018

cmarcelo commented Feb 13, 2018

kevincwells commented Feb 8, 2018 •

edited

kevincwells Feb 8, 2018 •

edited

cmarcelo left a comment •

edited

kevincwells commented Feb 9, 2018 •

edited

kevincwells commented Feb 10, 2018 •

edited

kevincwells commented Feb 12, 2018 •

edited