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

fix: poll npm pack until tarball is available #185

Merged
merged 5 commits into from
Nov 3, 2021
Merged

Conversation

cristiand391
Copy link
Member

@cristiand391 cristiand391 commented Oct 27, 2021

What does this PR do?

Adds polling logic to NpmModule, add new method fetchTarball that polls npm pack until the tarball is available.

Note:
plugin-rel-mgmt already polls npm view in the release process ,npm pack is the one failing right after npm view successfully retrieves the metadata.

Circle logs (signature verification step failed):
plugin-source
toolbelt

Depends on #182

What issues does this PR fix or reference?

@W-10065192@

Copy link
Contributor

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

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

comments are non-blocking, optional


public async pollForAvailability(checkFn: () => void): Promise<void> {
const ux = await UX.create();
const isNonTTY = process.env.CI !== undefined || process.env.CIRCLECI !== undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

is process.stdout.isTTY not reliable for CI environments?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should work too but haven't tried it, this command is executed by plugin-rel-mgmt.
CI and CIRCLE are built-in env vars in circle: https://circleci.com/docs/2.0/env-vars/#built-in-environment-variables

const stop = isNonTTY ? (msg: string): UX => ux.log(msg) : (msg: string): void => ux.stopSpinner(msg);

start('Polling for new version(s) to become available on npm');
while (!found && attempts < maxAttempts) {
Copy link
Contributor

Choose a reason for hiding this comment

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

there's a polling client in sfdx-core if you need that much, or the raw (approved) library here: https://github.com/normartin/ts-retry-promise

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried using the polling client in sfdx-core but ended up with the same code wrapped inside of it.
We still need to write the promise around this.pack and pass it to sfdx-core.

await this.pollForAvailability(() => {
this.pack(registry, options);
});
this.pack(registry, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

@cristiand391 Why are you calling this.pack twice in this function?

Also minor thing, the function name has "Tarball" misspelled

Copy link
Member Author

Choose a reason for hiding this comment

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

Since fetchTarball is called in src/lib/installationVerification.ts, every time I ran the tests the polling logic blocked the execution for minutes so to avoid that I moved the polling logic to pollForAvailability and called it in fetchTarball, that way it can be easily stubbed in tests to always resolve the promise.
The second call to this.pack should never be stubbed so it has to be called again after polling for tests to properly work.

@cristiand391 cristiand391 merged commit 001c1ab into main Nov 3, 2021
@cristiand391 cristiand391 deleted the npm-pack-polling branch November 3, 2021 20:56
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