Skip to content

refactor(auth): remove .env file import logic#183

Merged
pinglin merged 3 commits intomainfrom
pinglin/remove-env-for-auth
Sep 28, 2023
Merged

refactor(auth): remove .env file import logic#183
pinglin merged 3 commits intomainfrom
pinglin/remove-env-for-auth

Conversation

@pinglin
Copy link
Copy Markdown
Member

@pinglin pinglin commented Sep 26, 2023

Because

  • OAuth2 client-credential flow callback to a local machine can use a constant URL http://localhost:8085

  • client-id and client-secret for production api.instill.tech are injected during build/installation time by:

$ INSTILL_OAUTH_CLIENT_ID=<client-id> INSTILL_OAUTH_CLIENT_SECRET=<client-secret> make install

For internal maintenance only - We can still set staging and dev Instill Cloud with the corresponding client-id and client-secret through

$ ./bin/instill instances add <staging-api-url> \      
  --oauth2 <staging-auth-url> \
  --audience <staging-api-url> \
  --issuer <staging-api-url>/ \
  --client-secret <staging-client-secret> \
  --client-id <staging-client-id>

This commit

  • hardcode OAuth2 callback URL
  • remove unused global static variables for internal/oauth2
  • fix auth login right after installation error
  • fix missing refresh token

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 26, 2023

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (a9d3b4b) 51.09% compared to head (2db5038) 50.86%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #183      +/-   ##
==========================================
- Coverage   51.09%   50.86%   -0.24%     
==========================================
  Files          54       54              
  Lines        4196     4215      +19     
==========================================
  Hits         2144     2144              
- Misses       1833     1852      +19     
  Partials      219      219              
Flag Coverage Δ
unittests 50.86% <0.00%> (-0.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
pkg/cmd/auth/login/login.go 34.02% <0.00%> (-8.29%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TobiaszCudnik
Copy link
Copy Markdown
Contributor

This PR isnt a chore as it refactors a lot of existing code. Merging it at this point of the sprint isnt safe.

@TobiaszCudnik TobiaszCudnik force-pushed the main branch 4 times, most recently from 511098a to 8c90e7c Compare September 27, 2023 05:53
@pinglin pinglin changed the title chore(auth): remove .env file import logic refactor(auth): remove .env file import logic Sep 28, 2023
@pinglin pinglin force-pushed the pinglin/remove-env-for-auth branch from fac8347 to b7cb03a Compare September 28, 2023 11:50
@pinglin pinglin force-pushed the pinglin/remove-env-for-auth branch from b7cb03a to 826f23a Compare September 28, 2023 12:00
@pinglin pinglin force-pushed the pinglin/remove-env-for-auth branch from 51f688b to 2db5038 Compare September 28, 2023 17:45
@pinglin pinglin merged commit d01761f into main Sep 28, 2023
@pinglin pinglin deleted the pinglin/remove-env-for-auth branch September 28, 2023 17:49
@TobiaszCudnik
Copy link
Copy Markdown
Contributor

@pinglin This PR causes incorrect initial behavior when theres no config:

 !  ~/w/i/tmp  ./bin/instill instances list

  
    DEFAULT │ API HOSTNAME │ OAUTH2 HOSTNAME │ OAUTH2 AUDIENCE │ OAUTH2 ISSUER │ API VERSION  
  ──────────┼──────────────┼─────────────────┼─────────────────┼───────────────┼──────────────

Default instance logic has been broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants