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

Adding small changes to use docker layers for cache #1338

Merged
merged 1 commit into from Sep 6, 2019
Merged

Adding small changes to use docker layers for cache #1338

merged 1 commit into from Sep 6, 2019

Conversation

vsoch
Copy link
Contributor

@vsoch vsoch commented Aug 30, 2019

This pull request will pull sourcecred/sourcecred:latest first and then use the layers for cache as extra build args. @Beanow figured out all the logistics for sourcecred/widgets, and as promised, I'm following up on the merge of #1320.

Signed-off-by: Vanessa Sochat vsochat@stanford.edu

.circleci/config.yml Outdated Show resolved Hide resolved
@Beanow
Copy link
Contributor

Beanow commented Aug 30, 2019

🤔 still didn't cache. Is :latest that much out of date?

@vsoch
Copy link
Contributor Author

vsoch commented Aug 30, 2019

node:12 was updated a day ago - I can't tell how different it is though, at least from the Docker Hub interface. We'd have to have had a manifest with layers (from before) and now after.

@Beanow
Copy link
Contributor

Beanow commented Aug 30, 2019

node:12 was updated a day ago

That would explain. The layer hash is used to see if it needs to be invalidated for caching. How big of a difference doesn't matter.

Still think it's interesting, should we use :dev instead of :latest?
Because :latest here should be actual releases. We'll likely lose out a lot of caching chances especially when package.json or yarn.lock are updated on master.

@vsoch
Copy link
Contributor Author

vsoch commented Aug 30, 2019

I think it we target a more specific version of node:12, we would be more likely to get a cache. That said, if we target a more specific version, we probably won't get the same security updates. So - it's a tradeoff.

@Beanow
Copy link
Contributor

Beanow commented Aug 30, 2019

Agreed, I rather build a little longer than miss out on security updates.

So thinking about :dev to cache from a little more, we have these 3 build filters:

  • master
  • tag releases
  • other branches

master and tag releases should benefit from :dev
Other branches however runs most often and may sometimes not benefit, but most of the time will.

Main situations other branches wouldn't benefit I think.
When master has updated after the branch started. A rebase would fix that.
Or when the branch is not based off of master at all, for example a stable branch to maintain the previous major version. (Which we don't do)

But it would be a lot better whenever.
Master has updated since the last tag release (a lot of the time).

So I think it would give overall better results.
(Worth keeping in mind for widgets as well, though currently :latest and :dev are identical there)

@vsoch
Copy link
Contributor Author

vsoch commented Aug 30, 2019

okay so just to verify - you want to pull dev, to be added (used for cache) to build tag and master releases?

@Beanow
Copy link
Contributor

Beanow commented Aug 30, 2019

I think it should be all 3 on dev. So it can replace the :latest pull.

Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
@vsoch
Copy link
Contributor Author

vsoch commented Aug 30, 2019

"latest" changed to "dev"

@Beanow
Copy link
Contributor

Beanow commented Aug 30, 2019

Updated my comment for the record to avoid confusion. I think the other branches benefits a lot from :dev.

@teamdandelion teamdandelion merged commit ab0628a into sourcecred:master Sep 6, 2019
@teamdandelion
Copy link
Contributor

merged from @Beanow's approval. thanks! :)

Beanow pushed a commit that referenced this pull request Dec 20, 2019
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Beanow pushed a commit that referenced this pull request Dec 22, 2019
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
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.

None yet

3 participants