-
Notifications
You must be signed in to change notification settings - Fork 19
feat: support setting policy template via local file or restoring to default #366
Conversation
Similar to my comments in the other PR I would avoid adding this until we have proper Github App support. |
OK so do we want to (1) remove support for remote tracked policy templates or (2) not expose setting policy templates in the CLI at all? |
} | ||
} | ||
|
||
return sb.String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something that might be helpful is some sort of summary at the end. I have 200 workspaces in my dogfood, and it would be nice to see something like:
102 Workspaces will not be able to be rebuilt.
152 Workspaces have warnings but are still functional.
In the future, it would be great to merge-common errors and report which users are affected maybe? But for now I think a simple summary like that would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a summary at the end.
Mine isn't too exciting bc I regularly delete my workspaces in my dev deployment, so I only have like 2.
I'm curious to see what you think of the summary when there are 200 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very curious about why @Emyrk 200 workspaces 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, my deployment has 200 workspaces because I was testing the response times of our apis at those numbers. I made a little deployment branch to make them fast, but I never made anything to delete them in bulk haha.
I figure I'll just keep em as a bit of a "stress test" for the UI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The summary at 200 is not the most helpful thing, but I'm not worried about optimizing for 200 workspaces right now. Just thought a summary would be nice even for like ~30 workspaces which is reasonable at a company
} | ||
|
||
for _, mc := range resp.MergeConflicts { | ||
workspace, err := client.WorkspaceByID(ctx, mc.WorkspaceID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the extra details. In my dogfood of 200 workspaces this was still pretty quick 👍
Ah saw John's comment yea. Drop setting git tracked tracked templates. |
Yeah I initially didn't think this belonged in the CLI, but Ketan made a comment in the tls frontend CH ticket: "I think having the loose goal of CLI feature parity with the graphical web UI is a good default position." Then there's also this ticket which seems to imply we want this in the CLI |
coder-sdk/workspace.go
Outdated
|
||
if currentConflicts { | ||
if len(mc.CurrentTemplateWarnings) != 0 { | ||
sb.WriteString(fmt.Sprintf("Warnings: \n%s\n", strings.Join(mc.CurrentTemplateWarnings, "\n"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest using fmt.Fprintf(&sb
here https://play.golang.org/p/21-gij80d3c
} | ||
} | ||
|
||
return sb.String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very curious about why @Emyrk 200 workspaces 😂
No description provided.