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.

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

unified printing strategy #5334

Closed
wants to merge 1 commit into from
Closed

unified printing strategy #5334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 12 additions & 0 deletions contrib/completions/bash/oadm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,12 @@ _oadm_new-project()
flags+=("--description=")
flags+=("--display-name=")
flags+=("--node-selector=")
<<<<<<< HEAD
=======
flags+=("--output=")
two_word_flags+=("-o")
flags+=("--alsologtostderr")
>>>>>>> unified printing strategy
flags+=("--api-version=")
flags+=("--certificate-authority=")
flags_with_completion+=("--certificate-authority")
Expand Down Expand Up @@ -1038,6 +1044,12 @@ _oadm_groups_new()
flags_with_completion=()
flags_completion=()

<<<<<<< HEAD
=======
flags+=("--output=")
two_word_flags+=("-o")
flags+=("--alsologtostderr")
>>>>>>> unified printing strategy
flags+=("--api-version=")
flags+=("--certificate-authority=")
flags_with_completion+=("--certificate-authority")
Expand Down
12 changes: 12 additions & 0 deletions contrib/completions/bash/openshift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,12 @@ _openshift_admin_new-project()
flags+=("--description=")
flags+=("--display-name=")
flags+=("--node-selector=")
<<<<<<< HEAD
=======
flags+=("--output=")
two_word_flags+=("-o")
flags+=("--alsologtostderr")
>>>>>>> unified printing strategy
flags+=("--api-version=")
flags+=("--certificate-authority=")
flags_with_completion+=("--certificate-authority")
Expand Down Expand Up @@ -1593,6 +1599,12 @@ _openshift_admin_groups_new()
flags_with_completion=()
flags_completion=()

<<<<<<< HEAD
=======
flags+=("--output=")
two_word_flags+=("-o")
flags+=("--alsologtostderr")
>>>>>>> unified printing strategy
flags+=("--api-version=")
flags+=("--certificate-authority=")
flags_with_completion+=("--certificate-authority")
Expand Down
20 changes: 17 additions & 3 deletions pkg/cmd/admin/groups/new.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@ import (
"fmt"
"io"

"github.com/spf13/cobra"

kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
"k8s.io/kubernetes/pkg/util/sets"

"github.com/spf13/cobra"

"github.com/openshift/origin/pkg/client"
"github.com/openshift/origin/pkg/cmd/util/clientcmd"
"github.com/openshift/origin/pkg/cmd/util/mutation"
userapi "github.com/openshift/origin/pkg/user/api"
)

Expand All @@ -34,6 +35,9 @@ type NewGroupOptions struct {

Group string
Users []string

// MutationOutputOptions allows us to correctly output the mutations we make to objects in etcd
MutationOutputOptions mutation.MutationOutputOptions
}

func NewCmdNewGroup(name, fullName string, f *clientcmd.Factory, out io.Writer) *cobra.Command {
Expand All @@ -49,10 +53,14 @@ func NewCmdNewGroup(name, fullName string, f *clientcmd.Factory, out io.Writer)
kcmdutil.CheckErr(kcmdutil.UsageError(cmd, err.Error()))
}

options.MutationOutputOptions = mutation.NewMutationOutputOptions(f.Factory, cmd, out)

kcmdutil.CheckErr(options.AddGroup())
},
}

kcmdutil.AddOutputFlagsForMutation(cmd)
Copy link
Contributor

Choose a reason for hiding this comment

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

refactor this method upstream to bind to a variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deads2k what benefits from that change do I use to justify that PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@deads2k what benefits from that change do I use to justify that PR?

Currently, the printer and output methods on the Factory are very difficult to use during delegation flows. They rely on having a cmd set up with exactly the "right" flags. We should refactor the Factory to specify the actual values they need.

Having the proper object bound in will allow you separate the data from its expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deads2k thanks


return cmd
}

Expand Down Expand Up @@ -90,5 +98,11 @@ func (o *NewGroupOptions) AddGroup() error {
}

_, err := o.GroupClient.Create(group)
return err
if err != nil {
return err
}
if err := o.MutationOutputOptions.PrintSuccess(group, "created"); err != nil {
return err
}
return nil
}
7 changes: 7 additions & 0 deletions pkg/cmd/admin/groups/sync/cli/prune.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/openshift/origin/pkg/cmd/server/api"
"github.com/openshift/origin/pkg/cmd/server/api/validation"
"github.com/openshift/origin/pkg/cmd/util/clientcmd"
"github.com/openshift/origin/pkg/cmd/util/mutation"
)

const (
Expand Down Expand Up @@ -70,6 +71,9 @@ type PruneOptions struct {

// Out is the writer to write output to
Out io.Writer

// MutationOutputOptions allows us to correctly output the mutations we make to objects in etcd
MutationOutputOptions mutation.MutationOutputOptions
}

func NewPruneOptions() *PruneOptions {
Expand Down Expand Up @@ -97,6 +101,8 @@ func NewCmdPrune(name, fullName string, f *clientcmd.Factory, out io.Writer) *co
cmdutil.CheckErr(cmdutil.UsageError(c, err.Error()))
}

options.MutationOutputOptions = mutation.NewMutationOutputOptions(f.Factory, c, options.Out)

if err := options.Validate(); err != nil {
cmdutil.CheckErr(cmdutil.UsageError(c, err.Error()))
}
Expand Down Expand Up @@ -188,6 +194,7 @@ func (o *PruneOptions) Run(cmd *cobra.Command, f *clientcmd.Factory) error {

Out: o.Out,
Err: os.Stderr,
MutationOutputOptions: o.MutationOutputOptions,
}

listerMapper, err := getOpenShiftGroupListerMapper(clientConfig.Host(), o)
Expand Down
30 changes: 8 additions & 22 deletions pkg/cmd/admin/groups/sync/cli/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (

"github.com/spf13/cobra"

kapi "k8s.io/kubernetes/pkg/api"
cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
"k8s.io/kubernetes/pkg/runtime"
kerrs "k8s.io/kubernetes/pkg/util/errors"
Expand All @@ -27,8 +26,8 @@ import (
"github.com/openshift/origin/pkg/cmd/server/api"
configapilatest "github.com/openshift/origin/pkg/cmd/server/api/latest"
"github.com/openshift/origin/pkg/cmd/server/api/validation"
ocmdutil "github.com/openshift/origin/pkg/cmd/util"
"github.com/openshift/origin/pkg/cmd/util/clientcmd"
"github.com/openshift/origin/pkg/cmd/util/mutation"
)

const (
Expand Down Expand Up @@ -102,6 +101,9 @@ type SyncOptions struct {

// Out is the writer to write output to
Out io.Writer

// MutationOutputOptions allows us to correctly output the mutations we make to objects in etcd
MutationOutputOptions mutation.MutationOutputOptions
}

// CreateErrorHandler creates an error handler for the LDAP sync job
Expand Down Expand Up @@ -145,6 +147,8 @@ func NewCmdSync(name, fullName string, f *clientcmd.Factory, out io.Writer) *cob
cmdutil.CheckErr(cmdutil.UsageError(c, err.Error()))
}

options.MutationOutputOptions = mutation.NewMutationOutputOptions(f.Factory, c, options.Out)

if err := options.Validate(); err != nil {
cmdutil.CheckErr(cmdutil.UsageError(c, err.Error()))
}
Expand Down Expand Up @@ -357,6 +361,7 @@ func (o *SyncOptions) Run(cmd *cobra.Command, f *clientcmd.Factory) error {

Out: o.Out,
Err: os.Stderr,
MutationOutputOptions: o.MutationOutputOptions,
}

switch o.Source {
Expand Down Expand Up @@ -394,26 +399,7 @@ func (o *SyncOptions) Run(cmd *cobra.Command, f *clientcmd.Factory) error {
return err
}

// Now we run the Syncer and report any errors
openshiftGroups, syncErrors := syncer.Sync()
if o.Confirm {
return kerrs.NewAggregate(syncErrors)
}

list := &kapi.List{}
for _, item := range openshiftGroups {
list.Items = append(list.Items, item)
}
list.Items, err = ocmdutil.ConvertItemsForDisplayFromDefaultCommand(cmd, list.Items)
if err != nil {
return err
}

if err := f.Factory.PrintObject(cmd, list, o.Out); err != nil {
return err
}

return kerrs.NewAggregate(syncErrors)
return kerrs.NewAggregate(syncer.Sync())
}

func buildSyncBuilder(clientConfig ldapclient.Config, syncConfig *api.LDAPSyncConfig, errorHandler syncerror.Handler) (SyncBuilder, error) {
Expand Down
12 changes: 11 additions & 1 deletion pkg/cmd/admin/groups/sync/grouppruner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/openshift/origin/pkg/client"
"github.com/openshift/origin/pkg/cmd/admin/groups/sync/interfaces"
"github.com/openshift/origin/pkg/cmd/util/mutation"
)

// GroupPruner runs a prune job on Groups
Expand All @@ -33,6 +34,9 @@ type LDAPGroupPruner struct {
// Out is used to provide output while the sync job is happening
Out io.Writer
Err io.Writer

// MutationOutputOptions allows us to correctly output the mutations we make to objects in etcd
MutationOutputOptions mutation.MutationOutputOptions
}

var _ GroupPruner = &LDAPGroupPruner{}
Expand Down Expand Up @@ -79,7 +83,13 @@ func (s *LDAPGroupPruner) Prune() []error {
}
}

fmt.Fprintf(s.Out, "group/%s\n", groupName)
// The entire prune operation is not atomic, so we need to output what we do when we do it, so if we encounter
// a failure on another group the work we have already done is accounted for.
if s.DryRun {
s.MutationOutputOptions.PrintSuccessForDescription("Group", groupName, "would be deleted")
} else {
s.MutationOutputOptions.PrintSuccessForDescription("Group", groupName, "deleted")
}
}

return errors
Expand Down
2 changes: 2 additions & 0 deletions pkg/cmd/admin/groups/sync/grouppruner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/openshift/origin/pkg/client/testclient"
"github.com/openshift/origin/pkg/cmd/admin/groups/sync/interfaces"
"github.com/openshift/origin/pkg/cmd/util/mutation"
)

func TestGoodPrune(t *testing.T) {
Expand Down Expand Up @@ -121,6 +122,7 @@ func newTestPruner() (*LDAPGroupPruner, *testclient.Fake) {
Host: newTestHost(),
Out: ioutil.Discard,
Err: ioutil.Discard,
MutationOutputOptions: mutation.NewFakeOptions(),
}, tc

}
Expand Down
27 changes: 20 additions & 7 deletions pkg/cmd/admin/groups/sync/groupsyncer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,14 @@ import (
"github.com/openshift/origin/pkg/auth/ldaputil"
"github.com/openshift/origin/pkg/client"
"github.com/openshift/origin/pkg/cmd/admin/groups/sync/interfaces"
"github.com/openshift/origin/pkg/cmd/util/mutation"
userapi "github.com/openshift/origin/pkg/user/api"
)

// GroupSyncer runs a Sync job on Groups
type GroupSyncer interface {
// Sync syncs groups in OpenShift with records from an external source
Sync() (groupsAffected []*userapi.Group, errors []error)
Sync() (errors []error)
}

// LDAPGroupSyncer sync Groups with records on an external LDAP server
Expand All @@ -43,21 +44,23 @@ type LDAPGroupSyncer struct {
// Out is used to provide output while the sync job is happening
Out io.Writer
Err io.Writer

// MutationOutputOptions allows us to correctly output the mutations we make to objects in etcd
MutationOutputOptions mutation.MutationOutputOptions
}

var _ GroupSyncer = &LDAPGroupSyncer{}

// Sync allows the LDAPGroupSyncer to be a GroupSyncer
func (s *LDAPGroupSyncer) Sync() ([]*userapi.Group, []error) {
openshiftGroups := []*userapi.Group{}
func (s *LDAPGroupSyncer) Sync() []error {
var errors []error

// determine what to sync
glog.V(1).Infof("Listing with %v", s.GroupLister)
ldapGroupUIDs, err := s.GroupLister.ListGroups()
if err != nil {
errors = append(errors, err)
return nil, errors
return errors
}
glog.V(1).Infof("Sync ldapGroupUIDs %v", ldapGroupUIDs)

Expand Down Expand Up @@ -88,19 +91,29 @@ func (s *LDAPGroupSyncer) Sync() ([]*userapi.Group, []error) {
errors = append(errors, err)
continue
}
openshiftGroups = append(openshiftGroups, openshiftGroup)

if !s.DryRun {
fmt.Fprintf(s.Out, "group/%s\n", openshiftGroup.Name)
if err := s.updateOpenShiftGroup(openshiftGroup); err != nil {
fmt.Fprintf(s.Err, "Error updating OpenShift group %q for LDAP group %q: %v.\n", openshiftGroup.Name, ldapGroupUID, err)
errors = append(errors, err)
continue
}
}

// The entire sync operation is not atomic, so we need to output what we do when we do it, so if we encounter
// a failure on another group the work we have already done is accounted for.
if s.DryRun {
if err := s.MutationOutputOptions.PrintSuccess(openshiftGroup, "would be updated"); err != nil {
errors = append(errors, err)
}
} else {
if err := s.MutationOutputOptions.PrintSuccess(openshiftGroup, "updated"); err != nil {
errors = append(errors, err)
}
}
}

return openshiftGroups, errors
return errors
}

// determineUsers determines the OpenShift Users that correspond to a list of LDAP member entries
Expand Down
12 changes: 5 additions & 7 deletions pkg/cmd/admin/groups/sync/groupsyncer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/openshift/origin/pkg/auth/ldaputil"
"github.com/openshift/origin/pkg/client/testclient"
"github.com/openshift/origin/pkg/cmd/admin/groups/sync/interfaces"
"github.com/openshift/origin/pkg/cmd/util/mutation"
userapi "github.com/openshift/origin/pkg/user/api"
)

Expand Down Expand Up @@ -190,7 +191,7 @@ var Group3Members []*ldap.Entry = []*ldap.Entry{Member3, Member4}
// TestGoodSync ensures that data is exchanged and rearranged correctly during the sync process.
func TestGoodSync(t *testing.T) {
testGroupSyncer, tc := newTestSyncer()
_, errs := testGroupSyncer.Sync()
errs := testGroupSyncer.Sync()
for _, err := range errs {
t.Errorf("unexpected sync error: %v", err)
}
Expand All @@ -202,24 +203,20 @@ func TestListFails(t *testing.T) {
testGroupSyncer, _ := newTestSyncer()
testGroupSyncer.GroupLister.(*TestGroupLister).err = errors.New("error during listing")

groups, errs := testGroupSyncer.Sync()
errs := testGroupSyncer.Sync()
if len(errs) != 1 {
t.Errorf("unexpected sync error: %v", errs)

} else if errs[0] != testGroupSyncer.GroupLister.(*TestGroupLister).err {
t.Errorf("unexpected sync error: %v", errs)
}

if groups != nil {
t.Errorf("unexpected groups %v", groups)
}
}

func TestMissingLDAPGroupUIDMapping(t *testing.T) {
testGroupSyncer, tc := newTestSyncer()
testGroupSyncer.GroupLister.(*TestGroupLister).GroupUIDs = append(testGroupSyncer.GroupLister.(*TestGroupLister).GroupUIDs, "ldapgroupwithnouid")

_, errs := testGroupSyncer.Sync()
errs := testGroupSyncer.Sync()
if len(errs) != 1 {
t.Errorf("unexpected sync error: %v", errs)

Expand Down Expand Up @@ -333,6 +330,7 @@ func newTestSyncer() (*LDAPGroupSyncer, *testclient.Fake) {
Host: newTestHost(),
Out: ioutil.Discard,
Err: ioutil.Discard,
MutationOutputOptions: mutation.NewFakeOptions(),
}, tc

}
Expand Down
Loading