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

(fix) add repo meta create & update user-friendly error message#51985

Merged
erzhtor merged 7 commits into
mainfrom
erzhtor/add-repo-meta-create-update-user-friendly-error-messages
May 18, 2023
Merged

(fix) add repo meta create & update user-friendly error message#51985
erzhtor merged 7 commits into
mainfrom
erzhtor/add-repo-meta-create-update-user-friendly-error-messages

Conversation

@erzhtor

@erzhtor erzhtor commented May 16, 2023

Copy link
Copy Markdown
Contributor

Part of https://github.com/sourcegraph/pr-faqs/issues/96.

Test plan

Screenshots

Description Before After
Create duplicate key image image
Update non-existing key-value pair. This is mainly used by src repos update-metadata command image image

@erzhtor erzhtor requested review from a team, camdencheek, ryphil and toolmantim May 16, 2023 12:15
@erzhtor erzhtor self-assigned this May 16, 2023
@cla-bot cla-bot Bot added the cla-signed label May 16, 2023

@sashaostrikov sashaostrikov 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.

I proposed the other way around for checking that there is already an existing metadata key, PTAL

Comment thread internal/database/repo_kvps.go Outdated
@erzhtor erzhtor requested review from a team and sashaostrikov May 17, 2023 07:47
Comment thread internal/database/repo_kvps.go Outdated

@sashaostrikov sashaostrikov 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.

PTAL at my comment, I guess I caused some confusion with my last review, sorry!

Comment thread internal/database/repo_kvps.go Outdated
Comment thread internal/database/repo_kvps.go Outdated
@erzhtor erzhtor force-pushed the erzhtor/add-repo-meta-create-update-user-friendly-error-messages branch from 2bcec90 to e47b030 Compare May 18, 2023 08:49
@erzhtor erzhtor requested review from kopancek and sashaostrikov May 18, 2023 08:52

@sashaostrikov sashaostrikov 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.

:shipit:

@erzhtor erzhtor enabled auto-merge (squash) May 18, 2023 12:17
@erzhtor erzhtor disabled auto-merge May 18, 2023 12:17

@toolmantim toolmantim 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.

Much nicer!! Minor suggestion is to either remove "Repo", or use the full word "Repository" at the beginning and remove "for the given repository" at the end

Comment thread internal/database/repo_kvps.go Outdated
Comment thread internal/database/repo_kvps.go Outdated
erzhtor and others added 2 commits May 18, 2023 20:30
Co-authored-by: Tim Lucas <t@toolmantim.com>
Co-authored-by: Tim Lucas <t@toolmantim.com>
Comment thread internal/database/repo_kvps.go Outdated
Comment thread internal/database/repo_kvps.go Outdated
erzhtor and others added 2 commits May 18, 2023 22:37
Co-authored-by: Camden Cheek <camden@ccheek.com>
(chore): add scanKVP and scanKVPs reusable helper functions
@erzhtor erzhtor enabled auto-merge (squash) May 18, 2023 17:04
@erzhtor erzhtor disabled auto-merge May 18, 2023 17:20
@erzhtor erzhtor enabled auto-merge (squash) May 18, 2023 17:21
@erzhtor

erzhtor commented May 18, 2023

Copy link
Copy Markdown
Contributor Author

@kopancek, looks like I can't merge PR unless you stamp it?

@erzhtor erzhtor requested review from kopancek and removed request for kopancek May 18, 2023 17:22
@erzhtor erzhtor merged commit ef40256 into main May 18, 2023
@erzhtor erzhtor deleted the erzhtor/add-repo-meta-create-update-user-friendly-error-messages branch May 18, 2023 20:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants