cli core services setup and migration#873
Conversation
| return err | ||
| } | ||
| derivedOrganizationID, err := getOrganizationID(client) | ||
| ctx, err = context.GetCurrentContext() |
There was a problem hiding this comment.
@jemishp as we discussed, i simply read the org from context and send invite on behalf of the org without doing a reverse lookup. Let me know if this will be a problem
There was a problem hiding this comment.
Would it have an issue where the inviter's CLI does not have the correct OrgID in its org as the inviter may have been removed from the org?
Would core api catch that? I guess we cant test it out and refine as well.
There was a problem hiding this comment.
core api will error out in this case, also i think this is more align with user's exceptions which invite is sent on behalf of current org. We are good here.
| return err | ||
| } | ||
| // backfill OrganizationShortName | ||
| if (ctx.OrganizationShortName == "") && (ctx.Organization != "") { |
There was a problem hiding this comment.
here is the code that backfill organization_short_name in config
There was a problem hiding this comment.
just curious why would organization_short_name not be filled in because we already call c.SetOrganizationContext() with it if the user comes in via checkUserSession() or checkToken(), or via checkAPIKeys()
There was a problem hiding this comment.
because the backfill path in those place are optional.
- checkAPIKeys will only run in the context of api key.
- checkToken will not backfill if the token is valid as we only backfill short name in login path.
so a "catch all" migration step feels safer. I think in the future we will run into migration scenario as well so having a migration step will be helpful.
Codecov ReportBase: 87.29% // Head: 87.32% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #873 +/- ##
==========================================
+ Coverage 87.29% 87.32% +0.03%
==========================================
Files 108 109 +1
Lines 9326 9366 +40
==========================================
+ Hits 8141 8179 +38
Misses 697 697
- Partials 488 490 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| break | ||
| } | ||
| } | ||
| err = c.SetOrganizationContext(activeOrg.Id, activeOrg.ShortName) |
There was a problem hiding this comment.
This is guard against a rare use case where user's login org is different than the org he can access. We always treat AuthZ is the source of truth of user org relationship
missing1984
requested review from
AmFlint,
aliotta,
andriisoldatenko,
jemishp,
kushalmalani,
neel-astro and
sunkickr
as code owners
|
This is ready for review. i am adding new unit tests now. |
missing1984
changed the title
Co-authored-by: Jemish Patel <37133965+jemishp@users.noreply.github.com>
Co-authored-by: Jemish Patel <37133965+jemishp@users.noreply.github.com>
kushalmalani
left a comment
kushalmalani
left a comment
There was a problem hiding this comment.
Left a couple of comments..Code looks ok otherwise. We will be waiting to merge this in until 1.8 pre release is cut.
neel-astro
left a comment
neel-astro
left a comment
There was a problem hiding this comment.
left few minor comments, otherwise lgtm
|
could you make sure that the refresh tokens code still works. Code is located here: Line 149 in e6b73ff |
@sunkickr How to test it? I modified the exp date in config and rerun command -- a new token is fetched and saved. but i ran into an issue that the command which triggers the renew does not return anything. this is happening on main as well so i don't think it's introduced by my change. |
5 participants
Description
This PR migrated 3 endpoints to consume core services like UI does today.
3 commands are impacted by this change
astro organization list. - consume listOrganizations endpointastro user invite. - consume createUserInvite endpointastro login- consume both listOrganizations and getSelfUser endpointsCode details
Openapi codegen and core service client at
astro-client-core/*github.com/deepmap/oapi-codegenis brought in to generate the openapi client for core services. all generated code are inastro-client-core/api.gen.go.astro-client-core/client.gowrap up the integration of generated client and our context (setting auth headers)cmd/*layer changeastrocore.CoreClientfrom root and pass down to all cloud commandscloud/*layer change.Other notable change
orgShortNameis a required argument in order to call core services. currently we don't have it persisted in config. amigration.gomodule is introduced that backfill theorganization_short_namefield. the migration function will run at the end of the cloud setup function.astro-client-corecame with generated mocks. All relevant unit tests are switched over to use the new mocked client.also, this is my first golang PR so don't hesitate to comment out dumb mistake..
🎟 Issue(s)
https://github.com/astronomer/astro/issues/2247
🧪 Functional Testing
📸 Screenshots
📋 Checklist
make testbefore taking out of draftmake lintbefore taking out of draft