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

Allow passing in environment variables to the start command #152

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

rafiss
Copy link
Contributor

@rafiss rafiss commented Nov 11, 2022

No description provided.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@pawalt pawalt left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)


testserver/testservernode.go line 51 at r1 (raw file):

		"COCKROACH_TRUST_CLIENT_PROVIDED_SQL_REMOTE_ADDR=true",
		"COCKROACH_FORCE_DEV_VERSION=true",
		"COCKROACH_UPGRADE_TO_DEV_VERSION=true",

I'm sure I'm missing context here, but why do we always add the environment variables? Won't this force an upgrade even if the user doesn't want an upgrade to a dev version?


testserver/testservernode.go line 62 at r1 (raw file):

		wr, err := newFileLogWriter(fmt.Sprintf("%s.%d", ts.stdout, i))
		if err != nil {
			return fmt.Errorf("unable to open file %s: %w", ts.stdout, err)

Can you update this error message to reflect the new filename


testserver/testservernode.go line 71 at r1 (raw file):

		wr, err := newFileLogWriter(fmt.Sprintf("%s.%d", ts.stderr, i))
		if err != nil {
			return fmt.Errorf("unable to open file %s: %w", ts.stderr, err)

ditto

Copy link
Contributor Author

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @pawalt)


testserver/testservernode.go line 51 at r1 (raw file):

Previously, pawalt (Peyton Walters) wrote…

I'm sure I'm missing context here, but why do we always add the environment variables? Won't this force an upgrade even if the user doesn't want an upgrade to a dev version?

no prob - these env vars are new and also not too intuitive. essentially, the behavior is:

so they're just about what's allowed -- it doesn't actually force any upgrades

it's probably better for me to make a more general way of passing in env vars though. i'll do that instead.


testserver/testservernode.go line 62 at r1 (raw file):

Previously, pawalt (Peyton Walters) wrote…

Can you update this error message to reflect the new filename

i got rid of this change since it's unrelated to this PR and doesn't actually help with what i was trying to get it to do


testserver/testservernode.go line 71 at r1 (raw file):

Previously, pawalt (Peyton Walters) wrote…

ditto

same

@rafiss rafiss changed the title Allow upgrades from and to development versions Allow passing in environment variables to the start command Nov 14, 2022
@rafiss rafiss requested a review from pawalt November 14, 2022 14:59
Copy link
Contributor

@pawalt pawalt left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@pawalt
Copy link
Contributor

pawalt commented Nov 14, 2022

squirrel

@rafiss
Copy link
Contributor Author

rafiss commented Nov 14, 2022

thanks!

@rafiss rafiss merged commit 2828ea6 into cockroachdb:master Nov 14, 2022
@rafiss rafiss deleted the allow-dev-upgrades branch November 14, 2022 15:32
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.

None yet

3 participants