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

refactor: use MFE refs caching for master branches #163

Closed

Conversation

gabor-boros
Copy link
Contributor

@gabor-boros gabor-boros commented Nov 14, 2023

This PR fixes #161 by using the --keep-git-dir flag of the add command. The docker build step is cached if the branch is intact since the last build. However, if the branch is updated, the cache is invalidated and a new build is required -- as expected.

Screenshot 2023-12-04 at 13 38 50

The image above shows that the first build had ditch the previous cache; the second build shows that the image is rebuilt with no changes on the branch, therefore it is cached.

cc: @regisb @arbrandes

@gabor-boros gabor-boros marked this pull request as draft November 14, 2023 12:02
Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

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

I wish we would avoid calling the PROJECT_ROOT_READY and manually loading the config. Sorry about the back-and-forth, this feature is more complex than I expected...

tutormfe/plugin.py Outdated Show resolved Hide resolved
tutormfe/plugin.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

I'm missing something: how does this fix rate limiting if somebody is rebuilding off of master?

@regisb
Copy link
Contributor

regisb commented Nov 14, 2023

It wouldn't. But we expect that people and companies that face rate limiting issues don't build from master branches, but tags.

@arbrandes
Copy link
Collaborator

companies that face rate limiting issues don't build from master branches, but tags

Ah, this makes sense. @gabor-boros, do you mind including in this PR a change to the README that gives people this particular tip?

@gabor-boros
Copy link
Contributor Author

Reading back the comments, this raises a question for me: How would this solve the issue at all?

I mean, if the ADD command reaches out to GitHub to check if the cache need to be invalidated, we will hit the rate-limits again, but now only if the image is built for the master branches (many times). Right?

I guess we were not thinking about that @regisb. We were discussing to drop the caching like this during our call. Won't it make sense to do so?

@regisb
Copy link
Contributor

regisb commented Nov 21, 2023

I mean, if the ADD command reaches out to GitHub to check if the cache need to be invalidated, we will hit the rate-limits again, but now only if the image is built for the master branches (many times). Right?

Yes, this is correct. My reasoning is that this problem should not happen too often, if at all. And in particular, it should not affect your project. And when it does, you always have the option to create a plugin to remove the refs. Am I understanding this correctly?

@gabor-boros gabor-boros force-pushed the gabor/mfe-refs-for-master branch from 1825eaf to bb58c5a Compare December 4, 2023 09:40
@gabor-boros
Copy link
Contributor Author

@regisb This is now ready for your review. Based on our latest discussion, it seems that would be the ultimate solution. I think every needs will be satisfied with this changes as:

  • It is not calling out to API, hence no authentication is needed because of the rate limits
  • The caching still works as intended
  • The code is simplified
  • We added a feature with this change (as a side effect): private repositories could be cloned and cached as well

@gabor-boros gabor-boros requested a review from regisb December 4, 2023 09:46
@gabor-boros gabor-boros marked this pull request as ready for review December 4, 2023 09:47
Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

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

This is a breaking change, as it removes the "refs" attribute and requires a higher Docker version. As a consequence, please add a corresponding changelog entry with scriv create. Because this is a breaking change, we should only introduce it in the nightly branch, so you should rebase your branch on top of nightly. Finally, please update the README to remove references to "refs".

@gabor-boros
Copy link
Contributor Author

@regisb Sure thing. I'll do it by EOD

@arbrandes
Copy link
Collaborator

I think this would be good to get in Quince. Mind making those changes, @gabor-boros?

@regisb
Copy link
Contributor

regisb commented Dec 8, 2023

@gabor-boros if you don't have time that's fine too, I can make the change today. We're in a bit of a rush with the upcoming Quince release...

@gabor-boros
Copy link
Contributor Author

Hey @regisb, sorry, I ran out of time. If you can help out that would be awesome!

regisb pushed a commit that referenced this pull request Dec 8, 2023
See the extended conversation here:
#163

Close #161
@regisb
Copy link
Contributor

regisb commented Dec 9, 2023

Superseded by c4ed70c
Thanks a lot for your help @gabor-boros!

@regisb regisb closed this Dec 9, 2023
regisb added a commit to overhangio/tutor that referenced this pull request Jun 7, 2024
Similar to what we need with the MFE image, we leverage Docker's ADD
directive to implement caching of the edx-platform git checkout step.

See: overhangio/tutor-mfe#163
https://docs.docker.com/reference/dockerfile/#add
regisb added a commit to overhangio/tutor that referenced this pull request Jun 10, 2024
Similar to what we need with the MFE image, we leverage Docker's ADD
directive to implement caching of the edx-platform git checkout step.

See: overhangio/tutor-mfe#163
https://docs.docker.com/reference/dockerfile/#add
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

GitHub rate limit hit when running MFE image building in CI/CD pipelines
3 participants