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

Prevent npm --version from querying the internets for updates #6860

Merged
merged 6 commits into from Nov 8, 2019

Conversation

ujoni
Copy link
Contributor

@ujoni ujoni commented Nov 7, 2019

Fixes #6181


This change is Reviewable

Copy link
Contributor

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @caalador and @denis-anisimov)


flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 593 at r1 (raw file):

            npmVersionCommand.add("--version");
            FrontendVersion npmVersion = getVersion("npm", npmVersionCommand,
                    Collections.singletonMap("NO_UPDATE_NOTIFIER", "1"));

Shout this env var be set every time for npm call ?
What kind of updates it searches for ?
Update itself or module updates.
It looks like in the first case we should aways use it

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from External Reviews to Changes Requested Nov 7, 2019
Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)


flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 593 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

Shout this env var be set every time for npm call ?
What kind of updates it searches for ?
Update itself or module updates.
It looks like in the first case we should aways use it

Instead of adding a environment variable npm should only use the command argument --no-update-notifier

Copy link
Contributor Author

@ujoni ujoni left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)


flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 593 at r1 (raw file):

Previously, caalador wrote…

Instead of adding a environment variable npm should only use the command argument --no-update-notifier

Done.

Copy link
Contributor

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

denis-anisimov
denis-anisimov previously approved these changes Nov 7, 2019
Copy link
Contributor

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 1 unresolved discussion, 1 of 1 LGTMs obtained (waiting on @denis-anisimov)

denis-anisimov
denis-anisimov previously approved these changes Nov 7, 2019
Copy link
Contributor

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3.
Reviewable status: 1 unresolved discussion, 1 of 1 LGTMs obtained (waiting on @denis-anisimov)

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Changes Requested to Reviewer approved Nov 7, 2019
caalador
caalador previously approved these changes Nov 7, 2019
Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4.
Reviewable status: 1 unresolved discussion, 1 of 1 LGTMs obtained, and 1 stale (waiting on @denis-anisimov)

@ujoni ujoni added the +0.0.1 label Nov 7, 2019
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Reviewer approved to Changes Requested Nov 7, 2019
Copy link
Contributor

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

Does it fix #6181 ?
Could you please add Fixes #6181 then ?

Reviewed 1 of 1 files at r4, 1 of 1 files at r5.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained, and 2 stale

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Changes Requested to Reviewer approved Nov 7, 2019
@ujoni ujoni added +0.1.0 and removed +0.0.1 labels Nov 7, 2019
Copy link
Contributor

@mehdi-vaadin mehdi-vaadin left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5.
Reviewable status: :shipit: complete! all discussions resolved, 1 of 1 LGTMs obtained, and 2 stale

@ujoni ujoni added +0.0.1 and removed +0.1.0 labels Nov 8, 2019
@mehdi-vaadin mehdi-vaadin merged commit 1c98874 into master Nov 8, 2019
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Reviewer approved to Done - pending release Nov 8, 2019
@mehdi-vaadin mehdi-vaadin deleted the disable-npm-updates-checking branch November 8, 2019 07:43
ujoni added a commit that referenced this pull request Nov 8, 2019
* Prevent npm --version from querying the internets for updates

(cherry picked from commit 1c98874)
ujoni added a commit that referenced this pull request Nov 8, 2019
* Prevent npm --version from querying the internets for updates

(cherry picked from commit 1c98874)
ujoni added a commit that referenced this pull request Nov 8, 2019
* Prevent npm --version from querying the internets for updates

(cherry picked from commit 1c98874)
ujoni added a commit that referenced this pull request Nov 8, 2019
* Prevent npm --version from querying the internets for updates

(cherry picked from commit 1c98874)
@mehdi-vaadin mehdi-vaadin added this to the 2.2.0.alpha3 milestone Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
OLD Vaadin Flow ongoing work (Vaadin ...
  
Done - pending release
Development

Successfully merging this pull request may close these issues.

Goal prepare-package hangs in RC7
4 participants