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

replace go script with inline python code #3

Merged
merged 6 commits into from
Jan 21, 2024

Conversation

Borda
Copy link
Contributor

@Borda Borda commented Dec 14, 2023

trying to resolve #2

fyi, @jamesmortensen tested it on private repo and seems to be working correctly now

@Borda Borda changed the title adding checkout replace go script with inline python code Dec 14, 2023
@jamesmortensen
Copy link
Owner

Hi @Borda is there any way to keep the "prefix key" in the Python implementation? This is useful when needing to programmatically force a cache flush by having some control over the key manually. It's also going to be required to test the action. I think the main reason #4 was setup to run images from master is that CI is just a demo at this point. I'll address this in the comments in #4

@Borda
Copy link
Contributor Author

Borda commented Dec 15, 2023

is there any way to keep the "prefix key" in the Python implementation

Sure, I was really sure what it did as it was not part of the request call...
So now what the script does is walk one-by-one images and ask dockerhub when it was last updated... what then does the "prefix key" change on this pattern?

@Borda
Copy link
Contributor Author

Borda commented Dec 21, 2023

@jamesmortensen mind checking the updated version 🐿️

@Borda
Copy link
Contributor Author

Borda commented Jan 9, 2024

@jamesmortensen how is it going? 🦩

@jamesmortensen
Copy link
Owner

Hi @Borda really sorry for the delay in getting back to you. We just moved from one country to another and are still settling in.

The prefix-key in the action input serves two purposes:

  1. The prefix-key allows developers to bypass the cache in cases where they may need to force a fresh pull. Adding a prefix-key forces the cache key to be slightly different in order to "force" a cache miss and pull fresh data.

  2. The prefix key is also used in testing. In the cache-miss test, I purposely randomize the cache key to force a cache miss and verify that podman will pull the images and that there are no issues with this functionality.

Without the prefix key, the tests may pass the first time, since the cache may be empty, but if it is run immediately again, the tests will fail. In order to make them more repeatable, the cache key is randomized.

In order to make the cache-hit test more repeatable, I've added a prepopulate-cache job, which first pulls the images using the action. The cache-hit job depends on prepopulate-cache job. This ensures there is always a cache-hit and that we can verify this functionality passes.

Another use case for the prefix-key is if podman ever pulls or saves corrupted data. Forcing a cache-miss allows developers to force the action to pull a fresh Docker image instead of relying on what may be a corrupted cache.

Moreover, when I merged #2 I can see that the tests are failing.

You can see the behavior demonstrated here, where I tested against your fork. The first time, everything appears to pass, https://github.com/jamesmortensen/test-cache-container-images-action/actions/runs/7597163143, but without the prefix-key, during the second test, the cache-miss job fails, since the cache is populated with images from the previous test run: https://github.com/jamesmortensen/test-cache-container-images-action/actions/runs/7597179234

You gave me a few ideas on how to resolve the issue you were trying to solve, which is that the tests weren't failing and were only giving visible output to be interpreted manually.

What I've done is created a job called "prepopulate-cache", which pre-populates the cache by pulling the images using the same prefix-key that would be used in the cache-hit test. In other words, we pull the images, then we attempt to pull them again in another job and verify they are cached.

The cache-miss job still relies on a randomized prefix-key to ensure that the action cannot find any pulled images and will then verify that the "pull fresh images" logic works correctly. The tests are now repeatable from workflow run to workflow run, once I commit the changes back to master in this repository.

I'll also see if I can add uses: ./ back into the mix, so that what's being tested is actually the most recent changes and not the master branch.

Once I commit, I'll notify you here so you can resync from master. Then I think we can try testing the changes you've made from Python to Go.

@jamesmortensen
Copy link
Owner

One thing I want to also clarify is that the cache is also flushed if the Docker image is updated. The cache key is based on two things:

  1. The Prefix Key
  2. Last updated time of the image

So if the image is updated, the action pulls a fresh image, but if we need to do any testing, troubleshooting, or want to do a force cache miss for any reason, this is where the prefix key comes into play.

**TL;DR I think if you can modify the Python code to add the prefix key back to the container-images-key.txt file, then the prefix-key functionality will be preserved while trying to fix the bug you're attempting to fix in #2 **

What do you think?

@jamesmortensen
Copy link
Owner

Tests are passing in master: https://github.com/jamesmortensen/cache-container-images-action/actions and I don't see the error related to get-last-updated.go.

However, tests are not passing on your branch because we're unable to repeatedly force a cache miss. I added Borda:action/Checkout as the version for the action in this repo so you can see the differences in behavior: https://github.com/jamesmortensen/test-cache-container-images-action/actions/runs/7597532013/job/20692438981

For the bug #2 is there a way you can show me what you're facing in a public minrepro repository? If you try with the latest master, are you still facing that bug?

I'll try to be a bit more responsive going forward. Thank you and hope this helps!

@Borda
Copy link
Contributor Author

Borda commented Jan 21, 2024

One thing I want to also clarify is that the cache is also flushed if the Docker image is updated.

Yes, I saw that and this is preserved

@jamesmortensen
Copy link
Owner

jamesmortensen commented Jan 21, 2024 via email

Added back in the logic for adding an optional prefix-key for both testing the action in GitHub Actions CI workflows, as well as for forcing a cache flush in scenarios where it is needed.
@jamesmortensen jamesmortensen merged commit 5fa04ca into jamesmortensen:master Jan 21, 2024
3 checks passed
@jamesmortensen
Copy link
Owner

I modified the Python script to include the optional prefix-key. The tests also now pass. Thanks for converting the Go script to inline Python. I merged your pull request. I think it helps make the code more readable to avoid all of the bash backtick command substitution.

If you want to just confirm that pinning to master resolves the issues you were facing, I'll bump the version and make another release, @Borda

@Borda Borda deleted the action/Checkout branch January 22, 2024 06:39
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.

Error: stat ./get-last-updated.go: no such file or directory
2 participants