Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Add flux release --interactive #1231

Merged
merged 2 commits into from
Jul 23, 2018
Merged

Conversation

rndstr
Copy link
Contributor

@rndstr rndstr commented Jul 13, 2018

This adds an --interactive option to the release command that lets
you pick and choose which containers to update. It uses the new
containers specification in the /v9/update-manifest call.

Once the containers are selected, the release is sent off with
the constraint that the state of the containers need to be the exact
same as when the results were presented to the user. If anything changed
in the meanwhile, the whole release will not be processed.

Closes #1227

@rndstr rndstr requested a review from squaremo July 13, 2018 23:50
@squaremo
Copy link
Member

Running this (on Ubuntu 18.04, in a Terminal), I see:

$ fluxctl release --interactive --all --update-all-images
Submitting dry-run release...
Handling connection for 3030
   CONTROLLER   STATUS   UPDATES

Use [Space] to deselect containers and hit [Enter] to release selected.

then on :

panic: runtime error: integer divide by zero

goroutine 1 [running]:
github.com/weaveworks/flux/update.(*Menu).cursorUp(0xc42033f8c0)
        /home/mikeb/space/cloud/go/src/github.com/weaveworks/flux/update/menu.go:257 +0x67
github.com/weaveworks/flux/update.(*Menu).Run(0xc42033f8c0, 0xc42033f9e0, 0xc42033f700)
        /home/mikeb/space/cloud/go/src/github.com/weaveworks/flux/update/menu.go:181 +0x16c
main.promptSpec(0x932520, 0xc4200b4008, 0x0, 0x0, 0xc4202d6b80, 0xc42033f710, 0x0, 0x0, 0xc4202d6b80, 0xc42033f710, ...)
        /home/mikeb/space/cloud/go/src/github.com/weaveworks/flux/cmd/fluxctl/release_cmd.go:172 +0x5b
main.(*controllerReleaseOpts).RunE(0xc4201188f0, 0xc42010b680, 0xc42036ac00, 0x0, 0x3, 0x0, 0x0)
        /home/mikeb/space/cloud/go/src/github.com/weaveworks/flux/cmd/fluxctl/release_cmd.go:152 +0x9b4
main.(*controllerReleaseOpts).RunE-fm(0xc42010b680, 0xc42036ac00, 0x0, 0x3, 0x0, 0x0)
        /home/mikeb/space/cloud/go/src/github.com/weaveworks/flux/cmd/fluxctl/release_cmd.go:45 +0x52
github.com/weaveworks/flux/vendor/github.com/spf13/cobra.(*Command).execute(0xc42010b680, 0xc42036ab70, 0x3, 0x3, 0xc42010b680, 0xc42036ab70)
        /home/mikeb/space/cloud/go/src/github.com/weaveworks/flux/vendor/github.com/spf13/cobra/command.go:698 +0x46d
github.com/weaveworks/flux/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0xc42010ab40, 0xc42010ab40, 0x8fc9f0, 0x8fc900)
        /home/mikeb/space/cloud/go/src/github.com/weaveworks/flux/vendor/github.com/spf13/cobra/command.go:783 +0x2e4
main.run(0xc4200b6100, 0x4, 0x4, 0xc420094058)
        /home/mikeb/space/cloud/go/src/github.com/weaveworks/flux/cmd/fluxctl/main.go:14 +0x9d
main.main()
        /home/mikeb/space/cloud/go/src/github.com/weaveworks/flux/cmd/fluxctl/main.go:29 +0x63

The non-interactive fluxctl release also gives me an empty result, so I expect this is entirely about the case where there are no things to update.

@rndstr rndstr force-pushed the issue/1227-interactive-flux-release branch from 89e3740 to ce4b115 Compare July 16, 2018 19:16
@rndstr
Copy link
Contributor Author

rndstr commented Jul 16, 2018

@squaremo I knew there was something left to do.. fixed.

@squaremo
Copy link
Member

squaremo commented Jul 17, 2018

We don't currently build windows binaries for fluxctl, but it has been requested and does apparently build. It'd be a shame to undermine that.

Judging by the imports, I would expect the interactive stuff here to only work for linux; can you add a shim specific to Windows that at least says something like "Sorry, this doesn't work in Windows yet".

@squaremo
Copy link
Member

This doesn't build for windows (try adding windows to the list of OSes in the release-bins target in the Makefile), but it looks like it could in the future: pkg/term#8

I think you'll have to funnel the interactive bit through an OS-specific file, and make the windows one return an apologetic error.

@squaremo
Copy link
Member

On the UI itself, it is goodly simple and I like it.

It was not obvious to me what was going on to start off with, because my first attempt had only one change. But the instructions are clear enough (maybe you could mention arrow keys too). Is there a nice right-pointing arrow glyph, since we're glyphing?

There's some glitching:

$ fluxctl release -n hello --controller=deployment/helloworld --update-all-images --interactive                                                                                 
Submitting dry-run release...                                                                                                                                                   
Handling connection for 3030                                                                                                                                                    
   CONTROLLER              STATUS   UPDATESDATES                                                                                                                                
 ◉ hello:deployment/helloworld  success  helloworld: quay.io/weaveworks/helloworld:master-a000002 -> master-07a1b6b                                                             
>◯                                       sidecar: quay.io/weaveworks/sidecar:master-a000001 -> master-a000002                                                                   
                                                                                                                                                                                
Use [Space] to deselect containers and hit [Enter] to release selected. 

(the columns don't line up, and UPDATESDATES)

The "STATUS" column is a bit out of place -- what does "success" mean here? (I realise this is inherited from the dry run output, where it is also a bit odd).

Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

It works rather neatly.

The code looks fine to me -- I am a bit dubious of driving the result output and the menu through the same code, but I see why you've done it that way.

So, two changes before we can merge this:

  • fix up glitches (per the comment above)
  • put in a shim for Windows

@rndstr rndstr force-pushed the issue/1227-interactive-flux-release branch 3 times, most recently from 5a0d706 to fd41ce7 Compare July 20, 2018 23:21
@rndstr
Copy link
Contributor Author

rndstr commented Jul 20, 2018

I tried making it run in Windows but it needs a bit more work to read a single char cross-platform. existing libraries attempt to capture the whole terminal space by clearing it which i would rather not have.

maybe you could mention arrow keys too

doing so now 👍

Is there a nice right-pointing arrow glyph, since we're glyphing?

the columns don't line up, and UPDATESDATES

tabwriter didn't like the escape sequences.

The "STATUS" column is a bit out of place -- what does "success" mean here?

success means that it is updateable. if you provide --verbose you will also see the other statuses of updates that have not been selected for various reasons. i agree it looks odd while there is no other status.

@squaremo
Copy link
Member

success means that it is updateable. if you provide --verbose you will also see the other statuses of updates that have not been selected for various reasons. i agree it looks odd while there is no other status.

OK, ignore this one, it's not critical. (We can revisit how the results are shown, some other time)

@squaremo
Copy link
Member

squaremo commented Jul 23, 2018

image

  • I reckon put a space between the arrow and the toggle (possibly it's just my font scaling messing with things).
  • instead of "... to deselect containers" I suggest "... to toggle updates"

Sorry to drag this out :-/

(UPDATE: I reset font scaling and the arrow still overlaps the toggle)

@rndstr
Copy link
Contributor Author

rndstr commented Jul 23, 2018

@squaremo made the changes, PTAL

rndstr added 2 commits July 23, 2018 16:30
This adds an `--interactive` option to the `release` command that lets
you pick and choose which containers to update. It uses the new
`containers` specification in the /v9/update-manifest call.

Once the containers are selected, the release is sent off with
the constraint that the state of the containers need to be the exact
same as when the results were presented to the user. If anything changed
in the meanwhile, the whole release will not be processed.
@rndstr rndstr force-pushed the issue/1227-interactive-flux-release branch from 2b8f0a0 to 13b7c07 Compare July 23, 2018 23:33
@rndstr rndstr merged commit 25102d6 into master Jul 23, 2018
@rndstr rndstr deleted the issue/1227-interactive-flux-release branch July 23, 2018 23:37
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.

2 participants