Skip to content

Use new API to resolve workspaces server-side#801

Merged
eseliger merged 8 commits into
mainfrom
es/resolve-workspaces-server-side
Jul 19, 2022
Merged

Use new API to resolve workspaces server-side#801
eseliger merged 8 commits into
mainfrom
es/resolve-workspaces-server-side

Conversation

@eseliger

@eseliger eseliger commented Jul 13, 2022

Copy link
Copy Markdown
Member

Makes use of the newly introduced endpoint in src-cli. This is usually faster, and it means we can eventually have just one code path again.

I think this is good as-is and a net-improvement that we should get in, but before we can remove the old code path, we should have a conversation about the src batch repos command:

  • We cannot render the same output with the new resolver
  • Because the current output is incorrect. It doesn't consider workspaces, nor ignored repos or any potential overwrites by a later rule in the on: section
    Hence I propose that we shift this to run a proper workspace resolution and just print that outcome, like we do in the UI.

Closes https://github.com/sourcegraph/sourcegraph/issues/36261

Test plan

Validated everything works as expected.

Comment thread internal/batches/features.go Outdated
Comment on lines +251 to +257
if unsupported.HasUnsupported() {
return workspaces, repos, unsupported
}

if ignored.HasIgnored() {
return workspaces, repos, ignored
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This just mimics what we currently do here. This deserves a refactor, too, but not in this PR :)

@eseliger eseliger marked this pull request as ready for review July 15, 2022 13:50
@eseliger eseliger requested a review from a team July 15, 2022 13:50

@LawnGnome LawnGnome left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

In terms of src batch repos: yeah, given that was a pre-workspaces "eh, seems useful" thing, we should rethink that. Maybe we add a new src batch workspaces and make src batch repos a deprecated command that just does that with a message saying it's deprecated?

@courier-new courier-new left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agreed with your proposition and Adam's comment -- we can mark src batch repos as deprecated for now while we retain the old code path and have the proper resolution for src batch workspaces instead.

@eseliger eseliger merged commit dab9591 into main Jul 19, 2022
@eseliger eseliger deleted the es/resolve-workspaces-server-side branch July 19, 2022 11:58
scjohns pushed a commit that referenced this pull request Apr 24, 2023
Makes use of the newly introduced endpoint in src-cli. This is usually faster, and it means we can eventually have just one code path again.
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.

Add endpoint and use it for src-cli to resolve workspaces server-side

5 participants