-
Notifications
You must be signed in to change notification settings - Fork 952
chore: override codersdk.SessionTokenCookie in develop.sh #18991
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
Conversation
@@ -116,7 +116,7 @@ export default defineConfig({ | |||
secure: process.env.NODE_ENV === "production", | |||
}, | |||
}, | |||
allowedHosts: [".coder"], | |||
allowedHosts: [".coder", ".dev.coder.com"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review: also updated allowedHosts
for dev URLs.
scripts/develop.sh
Outdated
@@ -150,7 +151,7 @@ fatal() { | |||
trap 'fatal "Script encountered an error"' ERR | |||
|
|||
cdroot | |||
DEBUG_DELVE="${debug}" start_cmd API "" "${CODER_DEV_SHIM}" server --http-address 0.0.0.0:3000 --swagger-enable --access-url "${CODER_DEV_ACCESS_URL}" --dangerous-allow-cors-requests=true --enable-terraform-debug-mode "$@" | |||
DEBUG_DELVE="${debug}" CODER_DEV_SESSION_TOKEN_COOKIE_PREFIX="${CODER_DEV_SESSION_TOKEN_COOKIE_PREFIX}" start_cmd API "" "${CODER_DEV_SHIM}" server --http-address 0.0.0.0:3000 --swagger-enable --access-url "${CODER_DEV_ACCESS_URL}" --dangerous-allow-cors-requests=true --enable-terraform-debug-mode "$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review: I'm setting the prefix to dev_
by default in develop.sh
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it out and it is glorious we can run Coder over dev URLs now 🎉
Too bad we cannot get rid of codersdk.SessionTokenCookie
or set it dynamically (at least not as a const), but what can ya do.
codersdk/client.go
Outdated
func GetSessionTokenCookie() string { | ||
if buildinfo.IsDev() { | ||
if pfx, found := os.LookupEnv("CODER_DEV_SESSION_TOKEN_COOKIE_PREFIX"); found && pfx != "" { | ||
return pfx + "_" + SessionTokenCookie | ||
} | ||
} | ||
return SessionTokenCookie | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should SessionTokenCookie
just be a var
? And a function updates the var?
At the very least, SessionTokenCookie
should probably not be exported. Just this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was thinking of doing that but didn't want to make a breaking API change.
But you got me thinking... what do we do for buildinfo.tag
?
If I change SessionTokenCookie
to a var
we can just do the -ldflags -X
trick and not have to change anything else :)
woooooooooo!!!! |
// From codersdk/client.go | ||
export const SessionTokenCookie = "coder_session_token"; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't intentional; I need to figure out how to keep this around.
@Emyrk think it's possible to modify this behaviour in apitypings
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I'm manually vendoring this into api.ts
.
if [[ "$develop_in_coder" == 1 ]]; then | ||
echo "INFO : Overriding codersdk.SessionTokenCookie as we are developing inside a Coder workspace." | ||
ldflags+=( | ||
-X "'github.com/coder/coder/v2/codersdk.SessionTokenCookie=dev_coder_session_token'" | ||
) | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to try this way instead. It's a little more involved but requires less code changes in codersdk
. I can remove the echo message if it's annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!!
/** | ||
* Originally from codersdk/client.go. | ||
* The below declaration is required to stop Knip from complaining. | ||
* @public | ||
*/ | ||
export const SessionTokenCookie = "coder_session_token"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL
@@ -35,16 +37,20 @@ CODER_DEV_DIR="$(realpath ./.coderv2)" | |||
CODER_DELVE_DEBUG_BIN=$(realpath "./build/coder_debug_${GOOS}_${GOARCH}") | |||
popd | |||
|
|||
if [ -n "${CODER_AGENT_URL}" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also check if CODER=true
. That's what I do in my dotfiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot about that one!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also maybe be "${CODER_AGENT_URL:-}"
in case the env var is not set, on account of the set -u
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did that here above (L13) but missed it in develop.sh
. Will push a separate PR 👍
Enables viewing the Coder UI proxied by Coder.
Exposes an environment variable
CODER_DEV_SESSION_TOKEN_COOKIE_PREFIX
which allows adding a prefix to the name of the session token cookie. This only works for development builds.EDIT: I tried an alternative implementation instead using
-ldflags -X
that removes the need for most changes in codersdk.