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

Revert "cli, ui: dismiss release notes signup banner per environment ...variable" #57003

Merged
merged 1 commit into from
Nov 22, 2020
Merged

Revert "cli, ui: dismiss release notes signup banner per environment ...variable" #57003

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
1 change: 0 additions & 1 deletion pkg/cli/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,6 @@ go_test(
"//pkg/util",
"//pkg/util/bitarray",
"//pkg/util/duration",
"//pkg/util/envutil",
"//pkg/util/ipaddr",
"//pkg/util/json",
"//pkg/util/leaktest",
Expand Down
19 changes: 0 additions & 19 deletions pkg/cli/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"os/signal"
"path/filepath"
"runtime"
"strconv"
"strings"
"text/tabwriter"
"time"
Expand All @@ -37,7 +36,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/rpc"
"github.com/cockroachdb/cockroach/pkg/security"
"github.com/cockroachdb/cockroach/pkg/server"
"github.com/cockroachdb/cockroach/pkg/server/serverpb"
"github.com/cockroachdb/cockroach/pkg/server/status"
"github.com/cockroachdb/cockroach/pkg/server/telemetry"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
Expand Down Expand Up @@ -597,10 +595,6 @@ If problems persist, please see %s.`
if err := s.AcceptClients(ctx); err != nil {
return err
}
// Configure UI settings.
if err := setUIDataFromEnv(ctx, s); err != nil {
return err
}

// Now inform the user that the server is running and tell the
// user about its run-time derived parameters.
Expand Down Expand Up @@ -875,19 +869,6 @@ If problems persist, please see %s.`
return returnErr
}

// setUIDataFromEnv toggles presence of release notes signup banner
// based on an environment variable.
func setUIDataFromEnv(ctx context.Context, s *server.Server) error {
b := envutil.EnvOrDefaultBool("COCKROACH_UI_RELEASE_NOTES_SIGNUP_DISMISSED", false)
_, err := s.SetUIData(ctx, &serverpb.SetUIDataRequest{
KeyValues: map[string][]byte{"release_notes_signup_dismissed": []byte(strconv.FormatBool(b))}},
security.RootUserName())
if err != nil {
return err
}
return nil
}

// expandTabsInRedactableBytes expands tabs in the redactable byte
// slice, so that columns are aligned. The correctness of this
// function depends on the assumption that the `tabwriter` does not
Expand Down
58 changes: 0 additions & 58 deletions pkg/cli/start_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,12 @@
package cli

import (
"context"
"fmt"
"os"
"testing"

"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/rpc"
"github.com/cockroachdb/cockroach/pkg/server"
"github.com/cockroachdb/cockroach/pkg/server/serverpb"
"github.com/cockroachdb/cockroach/pkg/testutils"
"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
"github.com/cockroachdb/cockroach/pkg/util/envutil"
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/stretchr/testify/require"
)

func TestInitInsecure(t *testing.T) {
Expand Down Expand Up @@ -152,52 +143,3 @@ func TestAddrWithDefaultHost(t *testing.T) {
}
}
}

func TestSetUIFromEnv(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

ctx := context.Background()
s, _, _ := serverutils.StartServer(t, base.TestServerArgs{})
defer s.Stopper().Stop(ctx)
ts := s.(*server.TestServer)

const name = "COCKROACH_UI_RELEASE_NOTES_SIGNUP_DISMISSED"
const key = "release_notes_signup_dismissed"
for v, expected := range map[string]string{"true": "true", "false": "false", "": "false"} {
t.Run(fmt.Sprintf("%s=%s", name, v), func(t *testing.T) {
var err error
if v == "" {
err = os.Unsetenv(name)
} else {
err = os.Setenv(name, v)
}
if err != nil {
t.Fatal(err)
}
defer envutil.ClearEnvCache()

err = setUIDataFromEnv(ctx, ts.Server)
if err != nil {
t.Fatal(err)
}

cc, err := ts.RPCContext().GRPCDialNode(
ts.RPCAddr(),
1,
rpc.DefaultClass,
).Connect(ctx)
if err != nil {
t.Fatal(err)
}
adminClient := serverpb.NewAdminClient(cc)

resp, err := adminClient.GetUIData(ctx, &serverpb.GetUIDataRequest{Keys: []string{key}})
if err != nil {
t.Fatal(err)
}
actual := string(resp.KeyValues[key].Value)
require.Equal(t, expected, actual)
})
}
}
36 changes: 12 additions & 24 deletions pkg/server/admin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1194,12 +1194,17 @@ func splitUIKey(combined string) (string, string) {
return pair[0], pair[1]
}

// SetUIData stores the given key/value pairs in the
// SetUIData is an endpoint that stores the given key/value pairs in the
// system.ui table. See GetUIData for more details on semantics.
// This function writes to system.ui using the internal SQL executor of the provided server.
func (s *Server) SetUIData(
ctx context.Context, req *serverpb.SetUIDataRequest, userName security.SQLUsername,
func (s *adminServer) SetUIData(
ctx context.Context, req *serverpb.SetUIDataRequest,
) (*serverpb.SetUIDataResponse, error) {
ctx = s.server.AnnotateCtx(ctx)

userName, err := userFromContext(ctx)
if err != nil {
return nil, err
}

if len(req.KeyValues) == 0 {
return nil, status.Errorf(codes.InvalidArgument, "KeyValues cannot be empty")
Expand All @@ -1209,40 +1214,23 @@ func (s *Server) SetUIData(
// Do an upsert of the key. We update each key in a separate transaction to
// avoid long-running transactions and possible deadlocks.
query := `UPSERT INTO system.ui (key, value, "lastUpdated") VALUES ($1, $2, now())`
rowsAffected, err := s.sqlServer.internalExecutor.ExecEx(
rowsAffected, err := s.server.sqlServer.internalExecutor.ExecEx(
ctx, "admin-set-ui-data", nil, /* txn */
sessiondata.InternalExecutorOverride{
User: security.RootUserName(),
},
query, makeUIKey(userName, key), val)
if err != nil {
return nil, err
return nil, s.serverError(err)
}
if rowsAffected != 1 {
return nil, errors.Newf("rows affected %d != expected %d", rowsAffected, 1)
return nil, s.serverErrorf("rows affected %d != expected %d", rowsAffected, 1)
}
}

return &serverpb.SetUIDataResponse{}, nil
}

// SetUIData is an endpoint that stores the given key/value pairs in the
// system.ui table. See GetUIData for more details on semantics.
func (s *adminServer) SetUIData(
ctx context.Context, req *serverpb.SetUIDataRequest,
) (*serverpb.SetUIDataResponse, error) {
ctx = s.server.AnnotateCtx(ctx)
userName, err := userFromContext(ctx)
if err != nil {
return nil, err
}
_, err = s.server.SetUIData(ctx, req, userName)
if err != nil {
return nil, s.serverError(err)
}
return &serverpb.SetUIDataResponse{}, nil
}

// GetUIData returns data associated with the given keys, which was stored
// earlier through SetUIData.
//
Expand Down