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: identify scratch orgs if hub is known during auth #148

Merged
merged 28 commits into from
May 14, 2021

Conversation

mshanemc
Copy link
Contributor

@mshanemc mshanemc commented May 7, 2021

What does this PR do?

identify scratch orgs if hub is known during auth. Saves the value to devHubUsername so other commands (org:list, org:display, org:delete) can treat them like scratch orgs.

What issues does this PR fix or reference?

@W-9073698@
forcedotcom/cli#949
forcedotcom/cli#845

src/common.ts Outdated Show resolved Hide resolved
src/common.ts Outdated Show resolved Hide resolved
src/common.ts Outdated Show resolved Hide resolved
@mshanemc mshanemc requested a review from peternhale May 12, 2021 13:30
src/common.ts Show resolved Hide resolved
if (hubAuthInfos.length === 0) return;

// ask all those orgs if they know this orgId
await Promise.all(
Copy link
Contributor

Choose a reason for hiding this comment

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

@mshanemc feels like this could get expensive; API limits, time, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it scales with the number of devhubs you have. So for most users, fairly mild. We'd ask one hub about one ScratchOrgInfo.

If you did have a lot of hubs, we're still only one query per hub (re: limits) and executing in parallel (re: time)

What's a better alternative when the requirement is "at auth time, identify the hub for an org that may be a scratch org?"

Copy link
Contributor

Choose a reason for hiding this comment

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

@mshanemc no good answer really, maybe this will get better as we move towards sf, with a combined auth file.

src/common.ts Outdated Show resolved Hide resolved
await Promise.all(
(await AuthInfo.listAllAuthFiles())
.map((fileName) => basename(fileName, '.json'))
.map((username) => AuthInfo.create({ username }))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an expensive operation. Any alternatives?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't come up with any other way to figure out what auths are hubs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this approach limits the # of orgs that get queried in the above Promise.all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the fastest alternative would be to read the auth files directly (not go through core) but that opens up its own set of problems.

We could also add a method in core to deliver the raw auth file contents as json instead of AuthInfos. It's still potentially a lot of fs, but we could parallelize the read/parse better than core does today

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one more idea (kinda hacky) is to filter out obvious scratch-org-type names (foo@example.com, that thing where a timestamp is added to the main user to create secondary users, etc).

test/commands/auth/scratch-idenfity.nut.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@WillieRuemmele WillieRuemmele left a comment

Choose a reason for hiding this comment

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

I agree with Pete's comments

src/common.ts Outdated Show resolved Hide resolved
@mshanemc mshanemc requested a review from peternhale May 12, 2021 17:26
if (hubAuthInfos.length === 0) return;

// ask all those orgs if they know this orgId
await Promise.all(
Copy link
Contributor

Choose a reason for hiding this comment

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

@mshanemc no good answer really, maybe this will get better as we move towards sf, with a combined auth file.

@peternhale peternhale merged commit 3bd182a into main May 14, 2021
@peternhale peternhale deleted the sm/scratch-org-id branch May 14, 2021 17:38
peternhale pushed a commit that referenced this pull request May 14, 2021
* feat: identify scratch orgs if hub is known during auth

* chore: remove codecov

* chore: clean up github folder

* refactor: don't use prod-like loginUrl to exit

* chore: prune unnecessary ts-comment

* test: temporarily remove scratch-org-id to verify tests

* style: typos

* test: improve logging, temporarily use console

* chore: back to regular logging

* test: stubs for authinfo.hasAuthentications

* style: typos

* test: try single test

* test: return a promise?

* test: check authentications from instance

* test: stub method used by hasAuthentications

* test: run all the tests again

* test: try grant using stubbed listAll

* test: stub listAll for web:login

* test: store uses stub

* style: better error messages

* style: typo in nut name!

* refactor: changes from review comments

* refactor: getFields from authinfo in scratch-id

* Revert "refactor: getFields from authinfo in scratch-id"

This reverts commit c914648.

* style: comment for why we pass in the fields object

* fix: use decrypted fields, not result of getFields
WillieRuemmele pushed a commit that referenced this pull request Jun 11, 2021
* feat: add command auth:token:set

@W-8066452@

* chore: update command ref and topic map

* chore: unit tests and refactor from team guidance

* chore: update cmd ref and package.json

* chore: add tests

* chore: update command-snapshot

* chore: remove .codecov.yml

Authored via Leif

* chore: sync github PR slack notification (#152)

Authored via Leif

* feat: identify scratch orgs if hub is known during auth (#148)

* feat: identify scratch orgs if hub is known during auth

* chore: remove codecov

* chore: clean up github folder

* refactor: don't use prod-like loginUrl to exit

* chore: prune unnecessary ts-comment

* test: temporarily remove scratch-org-id to verify tests

* style: typos

* test: improve logging, temporarily use console

* chore: back to regular logging

* test: stubs for authinfo.hasAuthentications

* style: typos

* test: try single test

* test: return a promise?

* test: check authentications from instance

* test: stub method used by hasAuthentications

* test: run all the tests again

* test: try grant using stubbed listAll

* test: stub listAll for web:login

* test: store uses stub

* style: better error messages

* style: typo in nut name!

* refactor: changes from review comments

* refactor: getFields from authinfo in scratch-id

* Revert "refactor: getFields from authinfo in scratch-id"

This reverts commit c914648.

* style: comment for why we pass in the fields object

* fix: use decrypted fields, not result of getFields

* chore(release): 1.6.0 [ci skip]

* chore: sync github PR slack notification (#152)

Authored via Leif

* chore: add call to reestablish org full identity

* chore: set to fake return empty array

* chore: sync github PR slack notification (#152)

Authored via Leif

* chore: sync github PR slack notification (#152)

Authored via Leif

* chore: sync github PR slack notification (#152)

Authored via Leif

* feat: add accesstoken command tests

* chore: remove accesstokenfile parameter

* chore: requested review changes

* chore: clean up messages

* chore: update command example missing instanceurl

* chore: bump core version to 2.21.0

* chore: bump testkit to 1.1.2

* chore: dep changes

* chore: pin dev-scripts to 0.9.11

* chore: fix the tests

Co-authored-by: Benjamin Maggi <bmaggi@salesforce.com>
Co-authored-by: SF-CLI-BOT <72033612+SF-CLI-BOT@users.noreply.github.com>
Co-authored-by: Shane McLaughlin <shane.mclaughlin@salesforce.com>
Co-authored-by: SF-CLI-BOT <alm-cli@salesforce.com>
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.

3 participants