Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Conversation

@eseliger
Copy link
Member

@eseliger eseliger commented Jun 29, 2020

Closes #11193

image

Tasks from issue:

  • Move the IsRepoSupported checks so that patches from unsupported repositories are accepted, but not published (checks need to be moved to the Service and possibly the ExecChangesetJobs function)
  • In publishChangeset mutation we should return an error
  • In publishCampaignChangesets mutation we should skip them
  • We should add a Publishable: boolean property to Patch in the GraphQL API
  • We need to remove the "is supported" check in src-cli

For the last task: Do we really need to? We currently have a flag to allow creating patches for unsupported codehosts, which helps here. Removing it would mean we default to that behavior and I think that more customers would want to omit them than would want to store them for future use by default.

@codecov
Copy link

codecov bot commented Jun 29, 2020

Codecov Report

Merging #11793 into master will increase coverage by 0.00%.
The diff coverage is 92.00%.

@@           Coverage Diff           @@
##           master   #11793   +/-   ##
=======================================
  Coverage   47.68%   47.69%           
=======================================
  Files        1402     1402           
  Lines       79772    79790   +18     
  Branches     6732     6734    +2     
=======================================
+ Hits        38041    38057   +16     
- Misses      38155    38156    +1     
- Partials     3576     3577    +1     
Flag Coverage Δ
#go 51.85% <90.90%> (+<0.01%) ⬆️
#storybook 10.11% <ø> (ø)
#typescript 36.62% <100.00%> (+<0.01%) ⬆️
#unit 47.29% <92.00%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
cmd/frontend/graphqlbackend/campaigns.go 0.00% <ø> (ø)
...rc/enterprise/campaigns/detail/CampaignDetails.tsx 56.83% <ø> (ø)
web/src/enterprise/campaigns/detail/backend.ts 10.71% <ø> (ø)
...prise/campaigns/detail/patches/PatchSetPatches.tsx 80.00% <ø> (ø)
...erprise/internal/campaigns/resolvers/patch_sets.go 80.72% <60.00%> (-0.43%) ⬇️
enterprise/internal/campaigns/service.go 73.09% <100.00%> (-0.18%) ⬇️
enterprise/internal/campaigns/store.go 85.62% <100.00%> (+0.09%) ⬆️
.../enterprise/campaigns/detail/patches/PatchNode.tsx 72.41% <100.00%> (+2.04%) ⬆️
...odeintel/bundles/persistence/cache/reader_cache.go 93.75% <0.00%> (+2.08%) ⬆️

@mrnugget
Copy link
Contributor

For the last task: Do we really need to? We currently have a flag to allow creating patches for unsupported codehosts, which helps here. Removing it would mean we default to that behavior and I think that more customers would want to omit them than would want to store them for future use by default.

The ticket (https://github.com/sourcegraph/sourcegraph/issues/11193) says:

Right now, if you try to create a patch on a repo that is not supported (eg because it's on GitLab or another non-supported code host), it is silently discarded:

https://sourcegraph.com/github.com/sourcegraph/sourcegraph@615077bc91a6208bb60288fedba19a0adc5d6d1d/-/blob/enterprise/internal/campaigns/service.go#L82:17

This is surprising and prevents someone from trying out campaigns if their code host is not yet supported.

So, the question is: instead of silently dropping this, what options do we have?

@eseliger eseliger marked this pull request as ready for review June 29, 2020 15:23
@eseliger eseliger requested review from a team and emidoots as code owners June 29, 2020 15:23
@eseliger eseliger requested review from a team June 29, 2020 15:23
@eseliger
Copy link
Member Author

So, the question is: instead of silently dropping this, what options do we have?

This is fixed IMO:
The src-cli lists unsupported repos, and suggests to use the flag to still include them:
image

And once they hit the backend API, we will never discard them anymore, but keep them and display them as in the screenshot above, being publishable: false`. I think the ticket mentioned us dropping it silently on the backend, which we now don't do anymore.

@mrnugget
Copy link
Contributor

This is fixed IMO:

And that's also displayed when there were repositories found and not only in -v?

I think the ticket mentioned us dropping it silently on the backend

Both. From an end user's perspective you can't tell where it's dropped.

@eseliger
Copy link
Member Author

Ah you're right, it's not on non verbose mode.
what do you think about the below?

image

@mrnugget
Copy link
Contributor

what do you think about the below?

I think making it visible in non-verbose mode is good. I'm not sure yet about listing the repository names (because the list can grow quite long), but I think this is better than just a sentence.

The text itself contains a lot of redundancy: "(Including 1 on unsupported code hosts.)" should not be necessary if we print "The following repositories were filted out because [...]". But we can take a look at that in the src-cli PR.

Copy link
Contributor

@mrnugget mrnugget left a comment

Choose a reason for hiding this comment

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

Code looks good, but I think this can be simplified a lot in the backend. See my other comments.

@codecov
Copy link

codecov bot commented Jun 30, 2020

Codecov Report

Merging #11793 into master will increase coverage by 0.00%.
The diff coverage is 94.28%.

@@           Coverage Diff           @@
##           master   #11793   +/-   ##
=======================================
  Coverage   47.68%   47.69%           
=======================================
  Files        1402     1402           
  Lines       79772    79787   +15     
  Branches     6732     6776   +44     
=======================================
+ Hits        38041    38056   +15     
  Misses      38155    38155           
  Partials     3576     3576           
Flag Coverage Δ
#go 51.85% <93.75%> (+0.01%) ⬆️
#storybook 10.11% <ø> (ø)
#typescript 36.62% <100.00%> (+<0.01%) ⬆️
#unit 47.29% <94.28%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
cmd/frontend/graphqlbackend/campaigns.go 0.00% <ø> (ø)
...rc/enterprise/campaigns/detail/CampaignDetails.tsx 56.83% <ø> (ø)
web/src/enterprise/campaigns/detail/backend.ts 10.71% <ø> (ø)
...prise/campaigns/detail/patches/PatchSetPatches.tsx 80.00% <ø> (ø)
...erprise/internal/campaigns/resolvers/patch_sets.go 80.72% <60.00%> (-0.43%) ⬇️
enterprise/internal/campaigns/service.go 73.59% <100.00%> (+0.31%) ⬆️
.../enterprise/campaigns/detail/patches/PatchNode.tsx 72.41% <100.00%> (+2.04%) ⬆️
...odeintel/bundles/persistence/cache/reader_cache.go 93.75% <0.00%> (+2.08%) ⬆️

@eseliger
Copy link
Member Author

Related PR on src CLI: sourcegraph/src-cli#236

@eseliger eseliger removed the request for review from emidoots June 30, 2020 16:16
@eseliger eseliger merged commit b17f05a into master Jul 1, 2020
@eseliger eseliger deleted the es/preview-unsupported branch July 1, 2020 11:25
@eseliger eseliger mentioned this pull request Jul 6, 2020
28 tasks
@chrispine chrispine added the batch-changes Issues related to Batch Changes label Aug 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

batch-changes Issues related to Batch Changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow previewing of changesets for unsupported code hosts

4 participants