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 fetch-tags #5

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

rchamarthy
Copy link
Contributor

No description provided.

@mikemccracken
Copy link
Collaborator

nice, thanks! What do you think about making the fetch-tags just a flag instead of a separate action, so that ociv . can still be the default viewing action, and you don't need to remember ociv view ?

@rchamarthy
Copy link
Contributor Author

rchamarthy commented Dec 11, 2023

nice, thanks! What do you think about making the fetch-tags just a flag instead of a separate action, so that ociv . can still be the default viewing action, and you don't need to remember ociv view ?

Ok let me make it so. the problem is its not just one flag. Its 3 flags, registry, prefixes and output. We can eliminate output to be default ONLY. But I think we need the other two. So let me make them flags.

@rchamarthy
Copy link
Contributor Author

nice, thanks! What do you think about making the fetch-tags just a flag instead of a separate action, so that ociv . can still be the default viewing action, and you don't need to remember ociv view ?

Ok let me make it so. the problem is its not just one flag. Its 3 flags, registry, prefixes and output. We can eliminate output to be default ONLY. But I think we need the other two. So let me make them flags.

Done - Please review.

@mikemccracken
Copy link
Collaborator

just looked at the code in fetch - one thing that was good about the old python script is that it didn't fetch info for layers it already knew about - it read the known-layers file first and skipped any that were already there; this made updating a lot faster:

if f'{imagename}:{tag}' in existingImages:

if you want to add that later I'm fine with merging this, once we figure out the issue I raised separately with connection errors

@rchamarthy
Copy link
Contributor Author

just looked at the code in fetch - one thing that was good about the old python script is that it didn't fetch info for layers it already knew about - it read the known-layers file first and skipped any that were already there; this made updating a lot faster:

if f'{imagename}:{tag}' in existingImages:

if you want to add that later I'm fine with merging this, once we figure out the issue I raised separately with connection errors

I thought about it. I think we should actually not use cache. Because of retagging. A tag can be changed to a new SHA as per the distro specification so I decided not to cache. Let me know if you think otherwise.

@mikemccracken
Copy link
Collaborator

I thought about it. I think we should actually not use cache. Because of retagging. A tag can be changed to a new SHA as per the distro specification so I decided not to cache. Let me know if you think otherwise.

OK. I liked having a cache because of how long it took to get hundreds of image hashes, for the set of layers we have, but I agree that correct but slow is probably better for the public use.

maybe for future work we can have this loading go in the background and update the UI when done

it looks like the code as is will still use the file in ~/.cache/ociv if it exists and you don't pass -r -p right? that's a decent workaround for previous users

Ideally this would only query the registry for the few hashes we see, as we see them, but that requires e.g. zot's search extensions.

@rchamarthy
Copy link
Contributor Author

I thought about it. I think we should actually not use cache. Because of retagging. A tag can be changed to a new SHA as per the distro specification so I decided not to cache. Let me know if you think otherwise.

OK. I liked having a cache because of how long it took to get hundreds of image hashes, for the set of layers we have, but I agree that correct but slow is probably better for the public use.

maybe for future work we can have this loading go in the background and update the UI when done

it looks like the code as is will still use the file in ~/.cache/ociv if it exists and you don't pass -r -p right? that's a decent workaround for previous users

Ideally this would only query the registry for the few hashes we see, as we see them, but that requires e.g. zot's search extensions.

Yes. Thats what I thought. We cache and update as we start seeing drift or updates to the local repos. To make an improvement on perf, I added parallel processing. We can optimize that with persistent connections. For that, I need to look into the HTTP Client's caching capabilities.

@rchamarthy rchamarthy force-pushed the feat/fecth-tags branch 2 times, most recently from e3379a8 to 0d05895 Compare December 12, 2023 19:40
Signed-off-by: Ravi Chamarthy <[email protected]>
@mikemccracken mikemccracken merged commit 7e66bde into project-machine:main Dec 12, 2023
2 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.

2 participants