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

(feat) add "src repos {add|update|delete}-metadata -repo-name" flag support #977

Merged
merged 4 commits into from
Apr 27, 2023

Conversation

erzhtor
Copy link
Contributor

@erzhtor erzhtor commented Apr 26, 2023

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

This PR adds src repos {add|update|delete}-metadata -repo-name flag support

NOTE: When using repo-name flag, it makes an extra GQL query to get a repo ID. This is made for back compat purposes instead of introducing breaking change and updating underlying add/update/delete repo metadata GQL query.

Test plan

  • Add/update/delete using repo flag as previously
    • go run ./cmd/src repos add-metadata -repo=$repoID -key=test
    • go run ./cmd/src repos update-metadata -repo=$repoID -key=test -value=value
    • go run ./cmd/src repos delete-metadata -repo=$repoID -key=test
  • Add/update/delete using repo-name flag as previously
    • go run ./cmd/src repos add-metadata -repo-name=$repoName -key=test
    • go run ./cmd/src repos update-metadata -repo-name=$repoName -key=test -value=value
    • go run ./cmd/src repos delete-metadata -repo-name=$repoName -key=test

@erzhtor erzhtor self-assigned this Apr 26, 2023
@erzhtor erzhtor changed the title feat: add "src repos {add|update|delete}-metadata -repo-name" flag support (feat) add "src repos {add|update|delete}-metadata -repo-name" flag support Apr 26, 2023
Copy link
Member

@camdencheek camdencheek left a comment

Choose a reason for hiding this comment

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

nice!

@eseliger
Copy link
Member

The other commands seem to follow a different naming scheme, usually src ENTITY ACTION params so for example src teams create and such, should we align here?

@camdencheek
Copy link
Member

usually src ENTITY ACTION params

Is that not what this PR does? I'm reading a command like src repos add-metadata with ENTITY being "repos" and the ACTION being "add metadata"

@eseliger
Copy link
Member

Kinda, I thought that repo metadata would be the entity, but could be convinced otherwise 😀 src repo metadata add and so forth.

@erzhtor
Copy link
Contributor Author

erzhtor commented Apr 27, 2023

Kinda, I thought that repo metadata would be the entity, but could be convinced otherwise 😀 src repo metadata add and so forth.

Thanks for the feedback, @eseliger. TBH I haven't thought repo-metadata as an entity, but now that you've mentioned it, I think that does seem to make sense. On the other hand, though, repo metadata is part of a repo and can't exist independently, so both ways of thinking repos as an entity or repo-metadata as an entity could work.

For the scope of this PR, I would like to keep this as is, as changing will require more effort and refactoring to make it work both the new way and the old way for backward compat purposes.

@eseliger
Copy link
Member

That's okay 👍

@erzhtor erzhtor merged commit 3d540ec into main Apr 27, 2023
@erzhtor erzhtor deleted the erzhtor/add-repository-metadata-repo-name-flag-support branch April 27, 2023 12:15
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.

3 participants