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

Add request caching to Dockerfile generator #536

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

rassie
Copy link
Contributor

@rassie rassie commented Apr 22, 2024

When developing parts of the Dockerfile generation pipeline, and thus regenerating Dockerfiles over and over, many unnecessary HTTP requests are being made to https://api.adoptium.net, since the data is mostly unchanged. Adding a request cache with an expiration time of an hour allows this regeneration to run in under a second in most case, while not compromising on the correctness.

When developing parts of the Dockerfile generation pipeline, and thus regenerating Dockerfiles over and over, many
unnecessary HTTP requests are being made to https://api.adoptium.net, since the data is mostly unchanged. Adding a
request cache with an expiration time of an hour allows this regeneration to run in under a second in most case, while
not compromising on the correctness.
@karianna karianna requested review from gdams and tellison April 22, 2024 09:27
Copy link
Contributor

@tellison tellison left a comment

Choose a reason for hiding this comment

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

This will exacerbate the timing window already present in this logic, as the retrieval of the assets at the centre of the configuration/version/image_type loop is requested as the 'ga' which may change during the script run if you were unfortunate to be running while a Temurin release is underway - though I accept that is unlikely. The failure would be a skewed set of release images.

On the basis that this is run for production usage only beyond a Temurin release I'm happy to approve.

@tellison tellison merged commit e9d617a into adoptium:main Apr 22, 2024
70 checks passed
@gdams
Copy link
Member

gdams commented Apr 22, 2024

This will exacerbate the timing window already present in this logic, as the retrieval of the assets at the centre of the configuration/version/image_type loop is requested as the 'ga' which may change during the script run if you were unfortunate to be running while a Temurin release is underway - though I accept that is unlikely. The failure would be a skewed set of release images.

We could implement a parameter to skip cache? Then maybe say that for workflow_dispatch runs (e.g manual ones) it skips it? WDYT?

@rassie
Copy link
Contributor Author

rassie commented Apr 22, 2024

Hmm. I thought that workspace is cleaned before a build and thus the cache file is gone (i's in .gitignore), so that I assumed this patch would only be valid for local development, but not for CI. Do you keep files between builds?

@gdams
Copy link
Member

gdams commented Apr 22, 2024

Hmm. I thought that workspace is cleaned before a build and thus the cache file is gone (i's in .gitignore), so that I assumed this patch would only be valid for local development, but not for CI. Do you keep files between builds?

hmm yes this is a good point, this only affects local development. In which case I'm happy

@tellison
Copy link
Contributor

Yes, I'm happy to leave as is based on the original post that ...
"When developing parts of the Dockerfile generation pipeline, and thus regenerating Dockerfiles over and over..."

This would not affect the usual usage. I did momentarily consider whether it should have a guard only set for local development, but that seem unnecessary. Deploying for production and in GHA it is a single run through and we wouldn't be making repeated calls to the same API anyway.

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