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

Remove deprecated registry API #219

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

bart0sh
Copy link
Contributor

@bart0sh bart0sh commented Aug 8, 2024

No description provided.

@bart0sh bart0sh requested review from elezar and klihub August 8, 2024 15:21
@bart0sh
Copy link
Contributor Author

bart0sh commented Aug 8, 2024

This one is not as straightforward as the previous one, so @klihub please take a look. I'm not sure replacing GetRegistry with GetDefaultCache is correct approach.

@klihub
Copy link
Contributor

klihub commented Aug 8, 2024

This one is not as straightforward as the previous one, so @klihub please take a look. I'm not sure replacing GetRegistry with GetDefaultCache is correct approach.

It should be the correct approach. I'll take a closer look on Monday to see if I spot anything. But one thing I already noticed will need updating is our godoc in pkg/cdi/doc.go.

@bart0sh bart0sh mentioned this pull request Aug 9, 2024
18 tasks
@bart0sh bart0sh force-pushed the PR020-remove-registry-API branch from d534070 to b7c7dba Compare August 9, 2024 20:02
@bart0sh
Copy link
Contributor Author

bart0sh commented Aug 9, 2024

@klihub

But one thing I already noticed will need updating is our godoc in pkg/cdi/doc.go.

Thank you for pointing this out! I've updated the docs and code comments.

@bart0sh bart0sh force-pushed the PR020-remove-registry-API branch from b7c7dba to 6d88512 Compare August 9, 2024 20:04
@bart0sh
Copy link
Contributor Author

bart0sh commented Aug 23, 2024

@elezar @klihub Can you review this PR, please?

Copy link
Contributor

@elezar elezar left a comment

Choose a reason for hiding this comment

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

Thanks @bart0sh.

I have some comments regarding docs. I also think that when it makes sense to do so, we should rather instantiate a cache instance explicitly instead of relying on the (global) default cache. Especially in cases where we are configuring the spec directories.

cmd/cdi/cmd/root.go Outdated Show resolved Hide resolved
cmd/cdi/cmd/root.go Outdated Show resolved Hide resolved
cmd/cdi/cmd/root.go Outdated Show resolved Hide resolved
pkg/cdi/doc.go Outdated Show resolved Hide resolved
pkg/cdi/doc.go Outdated Show resolved Hide resolved
pkg/cdi/doc.go Outdated Show resolved Hide resolved
pkg/cdi/regressions_test.go Outdated Show resolved Hide resolved
@bart0sh bart0sh force-pushed the PR020-remove-registry-API branch from 6d88512 to b79ef0c Compare August 27, 2024 20:47
@bart0sh
Copy link
Contributor Author

bart0sh commented Aug 27, 2024

@elezar @klihub All review comments should be addressed now, please take a look again, thanks!

@bart0sh bart0sh force-pushed the PR020-remove-registry-API branch from b79ef0c to b89bae7 Compare August 27, 2024 20:58
@klihub klihub requested a review from elezar August 28, 2024 07:52
cmd/cdi/cmd/validate.go Outdated Show resolved Hide resolved
cmd/cdi/cmd/validate.go Outdated Show resolved Hide resolved
Copy link
Contributor

@klihub klihub left a comment

Choose a reason for hiding this comment

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

@bart0sh I have two very small nitpicks about help and info/error message consistency wrt. cache vs. Cache in messages. The original code was apparently inconsistent in this regard (no wonder, knowing the author), but this PR is already very close to fixing that with two exceptions.

Otherwise LGTM. Not a showstopper though, hence the approval.

@bart0sh bart0sh force-pushed the PR020-remove-registry-API branch from b89bae7 to 11fe536 Compare August 28, 2024 08:33
@bart0sh bart0sh force-pushed the PR020-remove-registry-API branch from 11fe536 to cddb43f Compare August 28, 2024 08:38
Signed-off-by: Evan Lezar <[email protected]>
Copy link
Contributor

@elezar elezar left a comment

Choose a reason for hiding this comment

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

I made some changes to the whitespace in doc.go.

Looks good otherwise.

pkg/cdi/doc.go Outdated Show resolved Hide resolved
@elezar elezar merged commit 96643f9 into cncf-tags:main Aug 28, 2024
7 checks passed
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