Skip to content
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

feat: use both project and user API token and populate default org id and projectid #325

Merged
merged 5 commits into from
Jan 30, 2024

Conversation

aayushrangwala
Copy link
Contributor

@aayushrangwala aayushrangwala commented Aug 8, 2023

Copy link
Contributor

@cprivitere cprivitere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to run much slower now than the current release. I assume that's due to it doing much more searching?

@cprivitere cprivitere temporarily deployed to external August 14, 2023 21:23 — with GitHub Actions Inactive
internal/init/init.go Outdated Show resolved Hide resolved
@cprivitere cprivitere changed the title Consider both project and user API token and populate default org id and projectid feat: use both project and user API token and populate default org id and projectid Oct 9, 2023
@cprivitere cprivitere temporarily deployed to external October 9, 2023 19:41 — with GitHub Actions Inactive
@cprivitere
Copy link
Contributor

So just tested this out, it's nearly 0 delay while waiting for it get your project id and org on the current main branch, and about 3-4 seconds on this new version. If we're ok with that, we are good to go.

exclude := []string{"devices", "members", "memberships", "invitations", "ssh_keys", "volumes", "backend_transfer_enabled", "updated_at", "customdata", "event_alert_configuration",
"timezone", "features", "avatar_url", "avatar_thumb_url", "two_factor_auth", "mailing_address", "max_projects", "verification_stage", "emails", "phone_number", "restricted",
"full_name", "email", "social_accounts", "opt_in_updated_at", "opt_in", "first_name", "last_name", "last_login_at"}
user, _, err := c.UserService.FindCurrentUser(context.Background()).Include(include).Exclude(exclude).Execute()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should stick with loading the current user first so that, if we were given a user token, we avoid loading more projects than necessary or making multiple paginated calls to the projects endpoint. What is the response and/or error that comes back if we try to run c.UserService.FindCurrentUser(context.Background()).Include(include).Exclude(exclude).Execute() with a project token? Is there something in the response/error that we can use to decide that the token might be a project token?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description of #265 had an approach in mind

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is already largely following the steps in the issue description. I still prefer adding project API support in a way that does not add latency to user token initialization, but adding the 2-project limit described in that issue to this PR would at least reduce the added latency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chris pointed out that a user might only have 1 project, in which case metal init would treat their token as a project token instead of a user token; the token type doesn't have any impact outside of the init command, though, so which endpoint we try to load first is largely a philosophical issue. I leave the decision up to the contributor as to whether we should remove auto-pagination from getProjectsOK or load the current user endpoint first and then try projects if current user 403s.

internal/init/init.go Outdated Show resolved Hide resolved
internal/init/init.go Outdated Show resolved Hide resolved
displague
displague previously approved these changes Jan 30, 2024
Copy link
Member

@displague displague left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a nitpick about the output which seems to be more of a debugging aid rather than necessary or helpful to end-users. https://github.com/equinix/metal-cli/pull/325/files#r1471550998

If this text were accompanied by a full list of organization ids that the key could access, it may be more useful. I would want to defer that work to accompany investigation into TUI or more interactive prompting tools (with name based autocompletes or console drop downs, for example).

Otherwise, this has been tested for a User API Key and Project API key. In my User testing I had a null project id default for some tests and a defined id - worked fine under both conditions.

aayushrangwala and others added 3 commits January 30, 2024 12:06
Signed-off-by: Chris Privitere <23177737+cprivitere@users.noreply.github.com>
Signed-off-by: Chris Privitere <23177737+cprivitere@users.noreply.github.com>
Signed-off-by: Chris Privitere <23177737+cprivitere@users.noreply.github.com>
Signed-off-by: Chris Privitere <23177737+cprivitere@users.noreply.github.com>
@cprivitere cprivitere merged commit bf56e76 into equinix:main Jan 30, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants