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

Task doesn't run on folder open anymore #160174

Closed
jrieken opened this issue Sep 6, 2022 · 24 comments · Fixed by #165570
Closed

Task doesn't run on folder open anymore #160174

jrieken opened this issue Sep 6, 2022 · 24 comments · Fixed by #165570
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug important Issue identified as high-priority insiders-released Patch has been released in VS Code Insiders regression Something that used to work is now broken tasks Task system issues verified Verification succeeded
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Sep 6, 2022

@jrieken
Copy link
Member Author

jrieken commented Sep 6, 2022

cc @alexr00 you fixed this in the past

@jrieken jrieken added the regression Something that used to work is now broken label Sep 6, 2022
@meganrogge meganrogge added bug Issue identified by VS Code Team member as probable bug tasks Task system issues labels Sep 6, 2022
@meganrogge meganrogge added this to the September 2022 milestone Sep 6, 2022
@meganrogge
Copy link
Contributor

I certainly caused this w the task reconnection - will look into it

@meganrogge
Copy link
Contributor

this is working for me now - likely fixed with one of my recent PRs related to automatic tasks

@jrieken jrieken reopened this Sep 27, 2022
@jrieken
Copy link
Member Author

jrieken commented Sep 27, 2022

Sorry, this doesn't work

@jrieken jrieken added the important Issue identified as high-priority label Sep 27, 2022
@meganrogge
Copy link
Contributor

@jrieken you need to allow automatic tasks to run. have you run
Screen Shot 2022-09-27 at 6 26 22 AM

@meganrogge
Copy link
Contributor

autotasks.mov

@meganrogge meganrogge added the info-needed Issue requires more information from poster label Sep 27, 2022
@jrieken
Copy link
Member Author

jrieken commented Sep 27, 2022

Screen.Recording.2022-09-27.at.15.29.13.mov

@jrieken
Copy link
Member Author

jrieken commented Sep 27, 2022

@jrieken you need to allow automatic tasks to run. have you run

Why is this now needed? I have trusted this workspace and I have run this exact task successfully in the past?

@meganrogge
Copy link
Contributor

it's always been needed. if you've ever hit disallow, it won't work. once you've allowed for the folder once, you shouldn't have to again

@jrieken
Copy link
Member Author

jrieken commented Sep 27, 2022

it's always been needed.

I doubt that's true. When I hit allow it writes a workspace setting and that wasn't needed, it came in via #154171. I have been using this feature years before this change - albeit infrequently in side projects.

I already have the "runOn": "folderOpen"- option and I have given trust to the folder. So, why do we put up further hurdles to use this feature?

@meganrogge
Copy link
Contributor

#154171 was added to prevent these tasks from running automatically without consent of the user in a workspace that may or may not have been set up /configured by them.

@meganrogge
Copy link
Contributor

some people can find it jarring and unwanted if they run automatically - even if they trust the workspace and happen to have that in their tasks.json file

@Tyriar
Copy link
Member

Tyriar commented Sep 27, 2022

If I'm remembering right there was always an opt-in via notification (maybe requiring engaging with the task system before it shows?). Whether a task runs on startup is a user preference, so instead of a hidden preference stored in local storage, tasks has moved to a setting. Is the problem that there was no migration to the setting?

@meganrogge
Copy link
Contributor

Is the problem that there was no migration to the setting?

Yes I think so

@jrieken
Copy link
Member Author

jrieken commented Sep 27, 2022

If I'm remembering right there was always an opt-in via notification

Looping in @alexr00 for reference but if I remember correctly this notification was the workaround for the lack of workspace trust. We prompted folks with an FYI to ask them for consent - similar prompts existed for eslint or tsserver and workspace trust is the answer

Whether a task runs on startup is a user preference,

No. This is a workspace configuration file which means "team preference". It is the same as using source maps for debugging or not. Or what typescript version to use etc. Also, the "Manage Automatic Tasks in Folder" writes a workspace setting not a user setting

@Tyriar
Copy link
Member

Tyriar commented Sep 27, 2022

Looping in @alexr00 for reference but if I remember correctly this notification was the workaround for the lack of workspace trust. We prompted folks with an FYI to ask them for consent

It was added here #64613, derived from this terminal change #23362. The discussion in the terminal feature request is here: #19758.

I did remember some of this wrong after digging into it and it was added primarily for security, but it's not the only reason for it now. The UX of this is an important part that was refined over time and carried over to terminal profiles; I have a preference of never wanting a workspace to modify my default terminal profile, regardless of whether I trust it.

No. This is a workspace configuration file which means "team preference".

@jrieken I disagree, I've always had auto run tasks disabled as I want to control when they run. This was a consideration when the notification was added. Source maps are a bad comparison as if they work they're almost always a better experience (though I often disable them to access ran variable names).

Also, the "Manage Automatic Tasks in Folder" writes a workspace setting not a user setting

Yes, we simplified it from opt-in to auto running tasks per workspace as that's handled by workspace trust now, to simply "am I a user who wants to enable auto run tasks?". That seems like it aligns with you wanting to have them always enabled and me wanted to have them always disabled.

@jrieken
Copy link
Member Author

jrieken commented Sep 27, 2022

I have a preference of never wanting a workspace to modify my default terminal profile, regardless of whether I trust it.

I also have many my preferences but the team preferences overrule them: source maps, prettier, lint rules etc pp. That's part of agreeing what are team and personal settings. As a side note: I am not aware that auto tasks have ever been a topic for us (VS Code) and I also don't see any of them in our task.json file. Don't get me wrong: I am happy with a "overrule team decision and go with my preference" path. I think this applies to a few none-critical things, like I want that for source maps.

Anyways, the implementation for auto-run task is poor:

  • no migration, essentially breaking the feature
  • the "manage automatic task in folder"-command writes a team/workspace-setting which defeats the purpose of overruling team configuration
  • the default value for task.allowAutomaticTasks is auto. Its docs say "Prompt for permission for each folder" which is not true and misleading

@meganrogge
Copy link
Contributor

meganrogge commented Sep 27, 2022

My initial feeling about this was if the folder is trusted, the tasks should run automatically when configured to do so.

Several points I encountered when I raised this were:

the manage automatic tasks command has been around for a long time and we didn't want to remove it for those that are used to it

The setting would make it easier and more discoverable to manage automatic tasks.

I am in favor of removing the prompt per folder and respecting 1) a trusted workspace 2) the setting as configured via the actions or directly.

I am also in favor of making this a user setting instead of a workspace/folder specific one.

@meganrogge meganrogge removed the info-needed Issue requires more information from poster label Sep 29, 2022
@meganrogge meganrogge removed this from the September 2022 milestone Sep 29, 2022
@meganrogge meganrogge added this to the October 2022 milestone Sep 29, 2022
@jrieken
Copy link
Member Author

jrieken commented Sep 29, 2022

of removing the prompt per folder and respecting 1) a trusted workspace 2) the setting as configured via the actions or directly.

I like that but the action shouldn't write a workspace-setting because task.json is already a "workspace setting". I would argue this should only be allowed as user-setting (which I believe our settings model supports). Also, the default should be on because you conflict team-choice - doing that shouldn't be default. In untrusted workspaces don't ever auto-run a task and maybe show a squiggle in task.json that signals that

@meganrogge
Copy link
Contributor

meganrogge commented Sep 29, 2022

Yes I meant as a user setting and agree with you re untrusted workspaces

I am in favor of making this a user setting instead of a workspace/folder specific one.

meganrogge added a commit that referenced this issue Oct 4, 2022
@meganrogge
Copy link
Contributor

We're bringing this to the UX sync to decide what the right approach is

@Tyriar Tyriar modified the milestones: October 2022, November 2022 Oct 24, 2022
@meganrogge
Copy link
Contributor

meganrogge commented Nov 2, 2022

Proposal:

Untrusted: ❌ automatic tasks
Trusted: ✅ based on user setting

@meganrogge
Copy link
Contributor

Approved in the UX sync

@meganrogge
Copy link
Contributor

Storage target will now be user and storage scope will be profile

meganrogge added a commit that referenced this issue Nov 4, 2022
@vscodenpa vscodenpa added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Nov 5, 2022
@sbatten sbatten added the verified Verification succeeded label Dec 1, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug important Issue identified as high-priority insiders-released Patch has been released in VS Code Insiders regression Something that used to work is now broken tasks Task system issues verified Verification succeeded
Projects
None yet
5 participants