-
Notifications
You must be signed in to change notification settings - Fork 82
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
Add eas env:push
and env:pull
commands
#2437
Add eas env:push
and env:pull
commands
#2437
Conversation
Size Change: +3.7 kB (+0.01%) Total Size: 52.5 MB
|
499c5d9
to
8bd1071
Compare
env:push
and env:pull
commands
8bd1071
to
db531a4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## piotrekszeremeta/eng-11913-add-eas-envcreate-command #2437 +/- ##
========================================================================================
- Coverage 52.94% 52.68% -0.25%
========================================================================================
Files 542 545 +3
Lines 20063 20247 +184
Branches 4101 4136 +35
========================================================================================
+ Hits 10620 10665 +45
- Misses 8630 8755 +125
- Partials 813 827 +14 ☔ View full report in Codecov by Sentry. |
2b270de
to
37c7b6a
Compare
05552ae
to
5aa373c
Compare
3eb0466
to
f8dc407
Compare
f821dbb
to
7c6f532
Compare
f8dc407
to
114acb6
Compare
e807c8d
to
70be519
Compare
710782b
to
63928f0
Compare
0365364
to
8d097bf
Compare
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 an expert on our auth/sudo system. Let's ask for the @wschurman review as well.
private async ssoSudoUpgradeAsync(): Promise<void> { | ||
throw Error('Sudo upgrade with SSO is not yet supported'); | ||
} |
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 need to figure out how to handle this case. We definitely want to allow SSO clients to pull env vars.
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.
Yep. This will need to do something similar to how SSO login is done in the CLI (using the website I believe). Can trace the SSO code in the CLI ssoLoginAsync
to see how SSO auth works. Happy to dig deeper if necessary.
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.
Would be good to stack a PR on top of this that adds SSO and then land them together.
f246d72
to
3ebe97d
Compare
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.
Since eas-cli doesn't have feature gating and does continuous deployment it'd probably be best to have the SSO functionality ironed out in a stacked PR before landing this. Otherwise we'd risk deploying a broken experience. Though I guess since the command is new and hidden it's okay for now, we can just fast-follow with the SSO fix.
private async ssoSudoUpgradeAsync(): Promise<void> { | ||
throw Error('Sudo upgrade with SSO is not yet supported'); | ||
} |
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.
Would be good to stack a PR on top of this that adds SSO and then land them together.
3ebe97d
to
50a54a8
Compare
9971a16
to
54af248
Compare
50a54a8
to
c66be42
Compare
54af248
to
6f6faf5
Compare
c66be42
to
621c5ad
Compare
6f6faf5
to
27d0d42
Compare
621c5ad
to
676c707
Compare
676c707
to
3f8749c
Compare
@wschurman @szdziedzic Ok, I'll work on SSO. I think the flow will be similar to that of login via SSO and I'll reuse some of its logic |
const { appVariables } = await EnvironmentVariablesQuery.byAppIdAsync( | ||
graphqlClient, | ||
projectId, | ||
const { appVariables } = await EnvironmentVariablesQuery.byAppIdAsync(graphqlClient, { |
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 we allow getting sensitive env vars values here as well?
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.
yes, we allow it here
.appVariables | ||
: await EnvironmentVariablesQuery.sharedAsync(graphqlClient, projectId); | ||
? ( | ||
await EnvironmentVariablesQuery.byAppIdAsync(graphqlClient, { |
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 we allow pulling sensitive env vars here as well?
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.
maybe with '--include-sensitive' flag? We'll save on decrypting values when user just want to know whether a variable is defined.
import SessionManager from './user/SessionManager'; | ||
|
||
// Handle the case where a user needs to be in sudo mode to perform an action. | ||
export async function withSudoModeAsync<T>( |
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 believe it doesn't respect --non-interactive
mode, right?
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.
No, it wouldn't make sense to automatically reauthorize in non-interactive mode.
❌ It looks like a changelog entry is missing for this PR. You have two options: you can add it manually, or you can use the changelog bot to do it for you. |
// Handle the case where a user needs to be in sudo mode to perform an action. | ||
export async function withSudoModeAsync<T>( | ||
sessionManager: SessionManager, | ||
graphqlQuery: () => Promise<T> | ||
): Promise<T> { | ||
try { | ||
return await graphqlQuery(); | ||
} catch (error: any) { | ||
const isSudoError = error.graphQLErrors?.some( | ||
(err: { extensions: { errorCode: string } }) => | ||
err?.extensions?.errorCode === 'SUDO_MODE_REQUIRED' | ||
); | ||
if (!isSudoError) { | ||
throw error; | ||
} | ||
await sessionManager.showSudoPromptAsync({ sso: false }); | ||
return await graphqlQuery(); | ||
} | ||
} |
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.
Offline discussion feedback:
It seems like requiring sudo mode is problematic for now, it doesn't work for SSO auth and doesn't seem to work with --non-interactive
mode, which is a big use case. For now, we decided to allow people to read sensitive env vars without sudo mode. This will require backend changes + dropping sudo logic from this PR.
1f07455
into
piotrekszeremeta/eng-11913-add-eas-envcreate-command
Why
User should be able to synchronise local environment variables with upstream.
How
eas env:pull
andeas env:push
commandseas env:pull
handles sudo access to request values of sensitive variablesTest Plan
Tested manually