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

cataloger rocks initial connect to catalog #976

Merged
merged 2 commits into from
Nov 29, 2020

Conversation

nopcoder
Copy link
Contributor

No description provided.

@nopcoder nopcoder requested a review from guy-har November 29, 2020 14:40
@nopcoder nopcoder self-assigned this Nov 29, 2020
@codecov-io
Copy link

Codecov Report

Merging #976 (950eec8) into master (4b28f2e) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #976      +/-   ##
==========================================
- Coverage   44.56%   44.55%   -0.01%     
==========================================
  Files         142      142              
  Lines       11547    11547              
==========================================
- Hits         5146     5145       -1     
- Misses       5758     5760       +2     
+ Partials      643      642       -1     
Impacted Files Coverage Δ
stats/collector.go 58.62% <0.00%> (-3.45%) ⬇️
catalog/mvcc/cataloger_create_entry.go 100.00% <0.00%> (+5.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b28f2e...950eec8. Read the comment docs.

Copy link
Contributor

@guy-har guy-har left a comment

Choose a reason for hiding this comment

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

Looks good

Comment on lines 403 to 405
if ns == "" {
return "", ErrInvalidStorageNamespace
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this validation enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

never enough - adding some code

Comment on lines 445 to 447
func NewCommitID(id string) (CommitID, error) {
return CommitID(id), nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

validate hex

Comment on lines 424 to 428
func NewRef(id string) (Ref, error) {
// TODO(barak): check with git what is a valid ref value
if !reValidBranchID.MatchString(id) {
return "", ErrInvalidRef
}
Copy link
Contributor

Choose a reason for hiding this comment

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

replace branchID verification with git ref verification( ~, ^, etc...)

@nopcoder nopcoder requested a review from guy-har November 29, 2020 16:27
@nopcoder nopcoder merged commit dc69cc4 into master Nov 29, 2020
@nopcoder nopcoder deleted the feature/cataloger-catalog branch December 2, 2020 06:36
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