-
Notifications
You must be signed in to change notification settings - Fork 73
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
refactor: try authenticating with codespaces on initial build #561
base: main
Are you sure you want to change the base?
Conversation
39e4939
to
987b474
Compare
https://github.com/electron/rbe-credential-helper/releases/tag/v0.3.0 is released so this should be unblocked now! |
987b474
to
0d0572a
Compare
function getTargetPlatform() { | ||
let targetPlatform = null; | ||
|
||
const arch = process.arch === 'arm64' ? 'arm64' : 'amd64'; |
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 know I wrote the old code, but isn't this wrong? We publish arm64 credential helpers I thought
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.
You're right, yeah: https://github.com/electron/rbe-credential-helper/actions/runs/8046443589
This is i assume saying "if it's x64 use amd64" though, no? so wouldn't that still be fine?
}); | ||
|
||
if (status !== 0) { | ||
if (process.env.CODESPACES) { |
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'm not sure this is what we want, on code spaces we should leave reclient enabled, just don't try login during init?
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.
meaning you'd prefer we maintain that state? i ended up just doing this as I figured it'd be easier
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.
After electron/electron#41428 landed, I don't think this PR is really necessary. On codespaces when you run e build
, it will now prompt:
Checking for build-tools updates
Running "git pull origin main --rebase --autostash" in /home/builduser/.electron_build_tools
From https://github.com/electron/build-tools
* branch main -> FETCH_HEAD
build-tools is up-to-date
Downloading "https://dev-cdn.electronjs.org/reclient/credential-helper/v0.2.2/electron-rbe-credential-helper-linux-amd64.tar.gz" into /home/builduser/.electron_build_tools/third_party/reclient.tar.gz
[===================================================================================================================================================================================] 77.57MB/s 100% 0.0s
Authentication Status: Not Authenticated
ERROR You do not have valid auth for Reclient, please run "e d rbe login"
builduser@c60311874b19:/workspaces/gclient/src/electron$
Which I believe is the preferred behavior
Refs electron/electron#41428 (comment).
Reworks reclient auth a bit to be more similar to goma in that if a user tries to build with reclient enabled, they will be prompted to log in. This also allows us to add some extra logic for codespaces so that we can auth during build instead of in. the oncreate lifecycle command.