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

Only trigger auto-update once in a VSCode session #623

Merged
merged 1 commit into from
May 17, 2021

Conversation

glennsarti
Copy link
Contributor

@glennsarti glennsarti commented May 4, 2021

Relates to #618

Previously the extension would trigger a new auto-update whenever it was
activated. However this can causes issues when a user deactivates-activates the
language server quickly multiple times, as it cause multiple timeouts to be
created. Additionally, when disabled, it would not clear any active timeouts.

This commit adds a small wrapper around the setTimeout function to track usage.
In particular, it will silently ignore calls to add a timeout if a timeout is
already active. This commit also clears any timeouts when the language server
is disabled.

Previously the extension would trigger a new auto-update whenever it was
activated. However this can causes issues when a user deactivates-activates the
language server quickly multiple times, as it cause multiple timeouts to be
created. Additionally, when disabled, it would not clear any active timeouts.

This commit adds a small wrapper around the setTimeout function to track usage.
In particular, it will silently ignore calls to add a timeout if a timeout is
already active.  This commit also clears any timeouts when the language server
is disabled.
@aeschright
Copy link
Contributor

This is a good start! I think that "Do not show again" button should be part of this change. We're also going to need a command for people to turn the updates back on, so they don't have to hunt through the settings.

@glennsarti
Copy link
Contributor Author

Would it be ok to log those as GH Issues and merge this PR as is?... Just afraid of putting too much change into a single PR.

@radeksimko
Copy link
Member

@glennsarti I agree that auto updates should only be triggered once and so the first change makes sense to me and that bug fix looks valuable.

However I feel the checkForUpdates is worth further discussion. Partially for the reasons that @aeschright mentioned - i.e. we do not want people to just turn off updates and forget about it, or make it too difficult to turn back on. Updates may contain security patches which are important.

Secondly I do wonder if the main motivation for this feature was the fact we'd show the popup way too often due to the bug that you fixed in #620 ? If not then I'd like to better understand the cases where people want to disable updates and reasons behind it.

Also see #628 which could provide a balance in a similar way the the Terraform ecosystem provides for providers where user declares constraint, such as ~> 1.0 and they then still continue receiving patch updates with bug fixes, but do not (indadvertedly) upgrade to a version that introduces breaking changes.

@glennsarti
Copy link
Contributor Author

Secondly I do wonder if the main motivation for this feature was the fact we'd show the popup way too often

Just to clarify, it fixes the bug when the VSCode is opened for a long time. The auto-update check still occurs every time the extension is activated.

But I do see your point though. And I do like #628. More elegant solution than just yes/no

I can think of some reasons why you want to disable updates:

  • You're always on a disconnected device so there's no point

  • Debugging terraform-ls, i.e. Trying out an older version and seeing if behaviour changes with the new version. In that case auto-update would stop using the old version

  • Corporate Proxies flagging the update URLs as "bad". This isn't strictly the extensions fault, but it's the one that causes it.

  • As an "Escape Hatch" if/when the update process is broken

Note - I'm not saying these are all valid reasons, but just things I can think of.


I think your suggestion in #628 addresses all the scenarios above. Personally, I wouldn't add commands to set/unset the version pin, and instead rely on extension documentation and VSCode's settings search UI to make it discoverable.

But I'm not a steward of this extension so I'm more than happy to take your lead on that.

@radeksimko
Copy link
Member

Thank you for adding all this context and listing these reasons @glennsarti - this is all very helpful!

The auto-update check still occurs every time the extension is activated.

I think that an attempt should be made - similar to how VS Code (and other software) makes attempts to update itself too, but presumably there are differences in how they handle errors?

You're always on a disconnected device so there's no point
Corporate Proxies flagging the update URLs as "bad". This isn't strictly the extensions fault, but it's the one that causes it.

These two I believe should be handled/ignored out of the box. i.e. the built-in logic should be able to tell that user is offline or that restrictive proxy is blocking requests - it shouldn't need to be told via config.

In case of user being off-line - preventing requests entirely would be best - I'm just not sure whether we can reliably detect it?

For any other network issues this gets a little bit murkier because we'd have to decide what kind of errors are worth surfacing to the user and which ones are left to be just silently logged, but I imagine that attempts should be made. I wonder how other extensions (e.g. Go extension w/ gopls) deal with this problem - do they silently ignore all network issues? That's also a valid option.

Debugging terraform-ls, i.e. Trying out an older version and seeing if behaviour changes with the new version. In that case auto-update would stop using the old version

#628 should be more elegant way of addressing that

As an "Escape Hatch" if/when the update process is broken

I'd like to think this one is rare enough that it doesn't need a switch. 🤔


Personally, I wouldn't add commands to set/unset the version pin, and instead rely on extension documentation and VSCode's settings search UI to make it discoverable.

That's fair - I think there are trade-offs, or rather use cases to consider. As LS always committed to semver, the user wouldn't be able to take full advantage of that with explicit version pinning.


Either way as you yourself mentioned it would be useful to keep this PR smaller, so how about we reduce it to that first commit, get it merged, and open a new issue to further discuss the above (offline mode, corporate proxies and other network issues) + continue conversation in #628 ?

@glennsarti glennsarti force-pushed the add-disable-update-check branch from 283b0d1 to 831c6a2 Compare May 17, 2021 08:20
@glennsarti
Copy link
Contributor Author

I'd like to think this one is rare enough that it doesn't need a switch.

Yup hopefully!

Either way as you yourself mentioned it would be useful to keep this PR smaller, so how about we reduce it to that first commit, get it merged,

Agreed. I've rebased with that commit dropped.

@glennsarti glennsarti changed the title Add checkForUpdates setting Only trigger auto-update once in a VSCode session May 17, 2021
@glennsarti
Copy link
Contributor Author

Changed PR description and title to match

@radeksimko radeksimko requested a review from aeschright May 17, 2021 10:53
Copy link
Contributor

@aeschright aeschright left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@aeschright aeschright merged commit 967ea7b into hashicorp:main May 17, 2021
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants