overhangio / tutor-mfe

This plugin makes it possible to easily add micro frontend (MFE) applications on top of an Open edX platform that runs with Tutor.
GNU Affero General Public License v3.0
22 stars 95 forks source link

refactor: use MFE refs caching for master branches #163

Closed gabor-boros closed 11 months ago

gabor-boros commented 1 year ago

This PR fixes https://github.com/overhangio/tutor-mfe/issues/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

regisb commented 1 year ago

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

arbrandes commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 11 months ago

@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:

gabor-boros commented 11 months ago

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

arbrandes commented 11 months ago

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

regisb commented 11 months ago

@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 commented 11 months ago

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

regisb commented 11 months ago

Superseded by https://github.com/overhangio/tutor-mfe/commit/c4ed70cf4e25a1c16b6945e2b73af47d82d31000 Thanks a lot for your help @gabor-boros!