Skip to content
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.

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

update to allow removing pending changes #856

Merged
merged 3 commits into from
Aug 1, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions client/changelist/changelist.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,24 @@ func (cl *memChangelist) Add(c Change) error {
return nil
}

// Remove deletes the changes found at the given indices
func (cl *memChangelist) Remove(idxs []int) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test for this method? I think the cmd functions only use file changelists which is why it isn't covered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

remove := make(map[int]struct{})
for _, i := range idxs {
remove[i] = struct{}{}
}
var keep []Change

for i, c := range cl.changes {
if _, ok := remove[i]; ok {
continue
}
keep = append(keep, c)
}
cl.changes = keep
return nil
}

// Clear empties the changelist file.
func (cl *memChangelist) Clear(archive string) error {
// appending to a nil list initializes it.
Expand Down
23 changes: 23 additions & 0 deletions client/changelist/changelist_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,26 @@ func TestMemChangeIterator(t *testing.T) {
var iterError IteratorBoundsError
require.IsType(t, iterError, err, "IteratorBoundsError type")
}

func TestMemChangelistRemove(t *testing.T) {
cl := memChangelist{}
it, err := cl.NewIterator()
require.Nil(t, err, "Non-nil error from NewIterator")
require.False(t, it.HasNext(), "HasNext returns false for empty ChangeList")

c1 := NewTUFChange(ActionCreate, "t1", "target1", "test/targ1", []byte{1})
cl.Add(c1)

c2 := NewTUFChange(ActionUpdate, "t2", "target2", "test/targ2", []byte{2})
cl.Add(c2)

c3 := NewTUFChange(ActionUpdate, "t3", "target3", "test/targ3", []byte{3})
cl.Add(c3)

err = cl.Remove([]int{0, 1})
require.NoError(t, err)

chs := cl.List()
require.Len(t, chs, 1)
require.Equal(t, "t3", chs[0].Scope())
}
33 changes: 27 additions & 6 deletions client/changelist/file_changelist.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ import (
"fmt"
"io/ioutil"
"os"
"path"
"sort"
"time"

"github.com/Sirupsen/logrus"
"github.com/docker/distribution/uuid"
"path/filepath"
)

// FileChangelist stores all the changes as files
Expand Down Expand Up @@ -46,13 +46,14 @@ func getFileNames(dirName string) ([]os.FileInfo, error) {
}
fileInfos = append(fileInfos, f)
}
sort.Sort(fileChanges(fileInfos))
return fileInfos, nil
}

// Read a JSON formatted file from disk; convert to TUFChange struct
func unmarshalFile(dirname string, f os.FileInfo) (*TUFChange, error) {
c := &TUFChange{}
raw, err := ioutil.ReadFile(path.Join(dirname, f.Name()))
raw, err := ioutil.ReadFile(filepath.Join(dirname, f.Name()))
if err != nil {
return c, err
}
Expand All @@ -70,7 +71,6 @@ func (cl FileChangelist) List() []Change {
if err != nil {
return changes
}
sort.Sort(fileChanges(fileInfos))
for _, f := range fileInfos {
c, err := unmarshalFile(cl.dir, f)
if err != nil {
Expand All @@ -89,10 +89,32 @@ func (cl FileChangelist) Add(c Change) error {
return err
}
filename := fmt.Sprintf("%020d_%s.change", time.Now().UnixNano(), uuid.Generate())
return ioutil.WriteFile(path.Join(cl.dir, filename), cJSON, 0644)
return ioutil.WriteFile(filepath.Join(cl.dir, filename), cJSON, 0644)
}

// Remove deletes the changes found at the given indices
func (cl FileChangelist) Remove(idxs []int) error {
fileInfos, err := getFileNames(cl.dir)
if err != nil {
return err
}
remove := make(map[int]struct{})
for _, i := range idxs {
remove[i] = struct{}{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we emit a warning or info message (but not fail) if i < 0 or > len(fileInfos)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we explicitly print the change #s, I've been thinking of this as one of those "silent success" cases. We do exactly what has been asked and technically silently succeed to "remove" the invalid indices.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with this silent success behavior for consistency across notary - we can always add in logging later if this is a pain-point for users

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense.

}
for i, c := range fileInfos {
if _, ok := remove[i]; ok {
file := filepath.Join(cl.dir, c.Name())
if err := os.Remove(file); err != nil {
logrus.Errorf("could not remove change %d: %s", i, err.Error())
}
}
}
return nil
}

// Clear clears the change list
Copy link
Contributor

Choose a reason for hiding this comment

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

The archive seems designed to save a copy of the changelist to the given directory, but since it has not been implemented yet, should we add a comment here.

// N.B. archiving not currently implemented
func (cl FileChangelist) Clear(archive string) error {
dir, err := os.Open(cl.dir)
if err != nil {
Expand All @@ -104,7 +126,7 @@ func (cl FileChangelist) Clear(archive string) error {
return err
}
for _, f := range files {
os.Remove(path.Join(cl.dir, f.Name()))
os.Remove(filepath.Join(cl.dir, f.Name()))
Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for fixing these to filepath!

}
return nil
}
Expand All @@ -121,7 +143,6 @@ func (cl FileChangelist) NewIterator() (ChangeIterator, error) {
if err != nil {
return &FileChangeListIterator{}, err
}
sort.Sort(fileChanges(fileInfos))
return &FileChangeListIterator{dirname: cl.dir, collection: fileInfos}, nil
}

Expand Down
3 changes: 3 additions & 0 deletions client/changelist/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ type Changelist interface {
// to save a copy of the changelist in that location
Clear(archive string) error

// Remove deletes the changes corresponding with the indices given
Remove(idxs []int) error

// Close syncronizes any pending writes to the underlying
// storage and closes the file/connection
Close() error
Expand Down
3 changes: 2 additions & 1 deletion client/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,11 @@ func applyChangelist(repo *tuf.Repo, cl changelist.Changelist) error {
default:
logrus.Debug("scope not supported: ", c.Scope())
}
index++
if err != nil {
logrus.Debugf("error attempting to apply change #%d: %s, on scope: %s path: %s type: %s", index, c.Action(), c.Scope(), c.Path(), c.Type())
return err
}
index++
}
logrus.Debugf("applied %d change(s)", index)
return nil
Expand Down
5 changes: 4 additions & 1 deletion cmd/notary/prettyprint.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ import (
"github.com/docker/notary/tuf/data"
)

const fourItemRow = "%s\t%s\t%s\t%s\n"
const (
fourItemRow = "%s\t%s\t%s\t%s\n"
fiveItemRow = "%s\t%s\t%s\t%s\t%s\n"
)

func initTabWriter(columns []string, writer io.Writer) *tabwriter.Writer {
tw := tabwriter.NewWriter(writer, 4, 4, 4, ' ', 0)
Expand Down
39 changes: 33 additions & 6 deletions cmd/notary/tuf.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,14 +97,22 @@ type tufCommander struct {
input string
output string
quiet bool

deleteIdx []int
reset bool
archiveChangelist string
}

func (t *tufCommander) AddToCommand(cmd *cobra.Command) {
cmdTUFInit := cmdTUFInitTemplate.ToCommand(t.tufInit)
cmdTUFInit.Flags().StringVar(&t.rootKey, "rootkey", "", "Root key to initialize the repository with")
cmd.AddCommand(cmdTUFInit)

cmd.AddCommand(cmdTUFStatusTemplate.ToCommand(t.tufStatus))
cmdStatus := cmdTUFStatusTemplate.ToCommand(t.tufStatus)
cmdStatus.Flags().IntSliceVarP(&t.deleteIdx, "unstage", "u", nil, "Numbers of changes to delete, as shown in status list")
cmdStatus.Flags().BoolVar(&t.reset, "reset", false, "Reset the changelist for the GUN by deleting all pending changes")
cmd.AddCommand(cmdStatus)

cmd.AddCommand(cmdTUFPublishTemplate.ToCommand(t.tufPublish))
cmd.AddCommand(cmdTUFLookupTemplate.ToCommand(t.tufLookup))

Expand Down Expand Up @@ -440,17 +448,36 @@ func (t *tufCommander) tufStatus(cmd *cobra.Command, args []string) error {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Thanks for fixing this formatting! It looks much nicer!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Big fan of tabwriter now that I've discovered it :-)


if t.reset {
return cl.Clear(t.archiveChangelist)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we have an interactive remove here? It would match behavior with removing keys and delegation roles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be nice to have a --reset --interactive but I don't think it's necessary for this PR. We have other things to get done right now :-)

}

if len(t.deleteIdx) > 0 {
return cl.Remove(t.deleteIdx)
}

if len(cl.List()) == 0 {
cmd.Printf("No unpublished changes for %s\n", gun)
return nil
}

cmd.Printf("Unpublished changes for %s:\n\n", gun)
cmd.Printf("%-10s%-10s%-12s%s\n", "action", "scope", "type", "path")
cmd.Println("----------------------------------------------------")
for _, ch := range cl.List() {
cmd.Printf("%-10s%-10s%-12s%s\n", ch.Action(), ch.Scope(), ch.Type(), ch.Path())
}
tw := initTabWriter(
[]string{"#", "ACTION", "SCOPE", "TYPE", "PATH"},
cmd.Out(),
)
for i, ch := range cl.List() {
fmt.Fprintf(
tw,
fiveItemRow,
fmt.Sprintf("%d", i),
ch.Action(),
ch.Scope(),
ch.Type(),
ch.Path(),
)
}
tw.Flush()
return nil
}

Expand Down
64 changes: 64 additions & 0 deletions cmd/notary/tuf_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@ package main
import (
"net/http"
"net/http/httptest"
"os"
"testing"

"github.com/spf13/cobra"
"github.com/spf13/viper"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -118,3 +121,64 @@ func TestAdminTokenAuthNon200Non401Status(t *testing.T) {
require.NoError(t, err)
require.Nil(t, auth)
}

func TestStatusUnstageAndReset(t *testing.T) {
setUp(t)
tempBaseDir := tempDirWithConfig(t, "{}")
defer os.RemoveAll(tempBaseDir)

tc := &tufCommander{
configGetter: func() (*viper.Viper, error) {
v := viper.New()
v.SetDefault("trust_dir", tempBaseDir)
return v, nil
},
}

tc.reset = true

// run a reset with an empty changelist and make sure it succeeds
err := tc.tufStatus(&cobra.Command{}, []string{"gun"})
require.NoError(t, err)

// add some targets
tc.sha256 = "88b76b34ab83a9e4d5abe3697950fb73f940aab1aa5b534f80cf9de9708942be"
err = tc.tufAddByHash(&cobra.Command{}, []string{"gun", "test1", "100"})
require.NoError(t, err)
tc.sha256 = "4a7c203ce63b036a1999ea74eebd307c338368eb2b32218b722de6c5fdc7f016"
err = tc.tufAddByHash(&cobra.Command{}, []string{"gun", "test2", "100"})
require.NoError(t, err)
tc.sha256 = "64bd0565907a6a55fc66fd828a71dbadd976fa875d0a3869f53d02eb8710ecb4"
err = tc.tufAddByHash(&cobra.Command{}, []string{"gun", "test3", "100"})
require.NoError(t, err)
tc.sha256 = "9d9e890af64dd0f44b8a1538ff5fa0511cc31bf1ab89f3a3522a9a581a70fad8"
err = tc.tufAddByHash(&cobra.Command{}, []string{"gun", "test4", "100"})
require.NoError(t, err)

out, err := runCommand(t, tempBaseDir, "status", "gun")
require.NoError(t, err)
require.Contains(t, out, "test1")
require.Contains(t, out, "test2")
require.Contains(t, out, "test3")
require.Contains(t, out, "test4")

_, err = runCommand(t, tempBaseDir, "status", "gun", "--unstage", "-1,1,3,10")

out, err = runCommand(t, tempBaseDir, "status", "gun")
require.NoError(t, err)
require.Contains(t, out, "test1")
require.NotContains(t, out, "test2")
require.Contains(t, out, "test3")
require.NotContains(t, out, "test4")

_, err = runCommand(t, tempBaseDir, "status", "gun", "--reset")
require.NoError(t, err)

out, err = runCommand(t, tempBaseDir, "status", "gun")
require.NoError(t, err)
require.NotContains(t, out, "test1")
require.NotContains(t, out, "test2")
require.NotContains(t, out, "test3")
require.NotContains(t, out, "test4")

}