Skip to content

roachprod: dismiss signup banner for nodes started by roachprod#56632

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
nkodali:20201112-roachprod-dismiss-banner
Nov 20, 2020
Merged

roachprod: dismiss signup banner for nodes started by roachprod#56632
craig[bot] merged 1 commit intocockroachdb:masterfrom
nkodali:20201112-roachprod-dismiss-banner

Conversation

@nkodali
Copy link
Copy Markdown
Collaborator

@nkodali nkodali commented Nov 12, 2020

Previously, when starting a node using roachprod, the db console
ui would display a release notes signup banner. This banner is not
useful for users of roachprod. We introduced a way to toggle this
via env var in #56437. This commit sets that
env var, COCKROACH_UI_RELEASE_NOTES_SIGNUP_DISMISSED=true for
cockroach start* commands issued from roachprod.

Resolves: #46998
See also: #56437

Release note: none

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

can you shorten the first commit line so it fits in the PR description?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @irfansharif, and @nkodali)


pkg/cmd/roachprod/install/cockroach.go, line 388 at r1 (raw file):

		export ROACHPROD=%[3]d%[4]s;
		GOTRACEBACK=crash COCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING=1 %[5]s \
		COCKROACH_UI_RELEASE_NOTES_SIGNUP_DISMISSED=true \

nit: not a blocker for me but is there any value to adding this var to whatever feeds h.getEnvVars() below? It gets inserted right into this spot: [5].

I don't know what the motivation is for putting other env vars in here directly or not.

@nkodali nkodali force-pushed the 20201112-roachprod-dismiss-banner branch from 47a4119 to b9af963 Compare November 12, 2020 23:39
Previously, when starting a node using roachprod, the db console
ui would display a release notes signup banner. This banner is not
useful for users of roachprod. We introduced a way to toggle this
via env var in cockroachdb#56437. This commit sets that
env var, COCKROACH_UI_RELEASE_NOTES_SIGNUP_DISMISSED=true for
`cockroach start*` commands issued from roachprod.

Resolves: cockroachdb#46998
See also: cockroachdb#56437

Release note: none
@nkodali nkodali force-pushed the 20201112-roachprod-dismiss-banner branch from b9af963 to 8611b71 Compare November 13, 2020 18:28
Copy link
Copy Markdown
Collaborator Author

@nkodali nkodali left a comment

Choose a reason for hiding this comment

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

Shortened commit msg first line.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian and @irfansharif)


pkg/cmd/roachprod/install/cockroach.go, line 388 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

nit: not a blocker for me but is there any value to adding this var to whatever feeds h.getEnvVars() below? It gets inserted right into this spot: [5].

I don't know what the motivation is for putting other env vars in here directly or not.

Good catch. I initially thought h.getEnvVars() only got args from os.Environ to forward any env vars set on a roachprod command to the cockroach start command. But looks like it's also getting default args from main.nodeEnv via SyncedCluster.Env. Added this to main.nodeEnv.

@nkodali nkodali requested a review from dhartunian November 18, 2020 17:19
Copy link
Copy Markdown
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

:lgtm: just update the PR title/message to match commit :) that's one unfortunate consequence of editing commit messages in revisions.

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian)

@nkodali nkodali changed the title roachprod: dismiss signup release banner on db console ui for all nod… roachprod: dismiss signup banner for nodes started by roachprod Nov 20, 2020
@nkodali
Copy link
Copy Markdown
Collaborator Author

nkodali commented Nov 20, 2020

bors r=dhartunian,irfansharif

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Nov 20, 2020

Build succeeded:

@craig craig bot merged commit 9806c1c into cockroachdb:master Nov 20, 2020
@nkodali nkodali deleted the 20201112-roachprod-dismiss-banner branch November 24, 2020 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ui: Allow the release notes signup banner to be disabled by an environment variable

4 participants