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

chore(tests) include kong*.rockspec to cache hash calculation on travis #6380

Closed
wants to merge 1 commit into from

Conversation

bungle
Copy link
Member

@bungle bungle commented Sep 22, 2020

Summary

If it is not included in cache hash calculation, dependencies that are updated in .rockspec might be leaked to other tests / builds.

@dndx
Copy link
Member

dndx commented Sep 22, 2020

I would like to hear more opinions from others, for the following reasons:

  1. Every time before the tests are ran, make dev is executed, which will correct all the dependency issue, if any.
  2. Adding rockspec makes the cache granularity unnecessarily large. This means even if we are just adding new Kong lua files to the rockspec, a full rebuild is triggered, which is very slow and reduces our cache hit rate.

Basically, everything that can not be corrected by make dev are inside the .requirments file, which is part of the hash as f today. Everything else should be correctable by make dev much quicker than a full rebuild.

Obviously, being defensive is a good thing, for the CI runs the most defensive thing we could have done is to not use cache at all. We choose to not do that because it creates more harm without solving any known issues for us, the same principle should apply here as well.

@bungle
Copy link
Member Author

bungle commented Sep 22, 2020

Basically, everything that can not be corrected by make dev are inside the .requirements

We tried couple of things. But didn't find the solution that is better than this. Could we go with this solution as it makes cache correct, and then try to optimize it later? If you have better way to solve this, I am open to see it (make dev should not just go and purge developers luarocks, so it is a bit hard to implement correctly). Here is a sister PR to this that is for next and it is green on travis: #6383 (so this fix really works). The Github Actions fix can be found on kong-build-tools.

If cache gets correct, everything will suddenly turn green, so you never know do you have this: #6361 screwing up the cache. It depends that does cache have that or almost any other branch based on next.

I don't agree this creates more harm. It does similar thing to editing .requirements. So editing Makefile or .rockpec does use different cache (just like editing .requirements). But those are edited rarely.

@kikito
Copy link
Member

kikito commented Sep 23, 2020

We could only hash the rockspec dependencies, but not when adding a new file to Kong (or when adding a blank line to the rockspec).

resty -e "setmetatable(_G,nil); assert(loadfile('kong-2.1.3-0.rockspec'))(); table.sort(dependencies); for _,d in ipairs(dependencies) do print(d) end"

Output looks like:

inspect == 3.1.1
kong-lapis == 1.8.1.2
kong-plugin-acme ~> 0.2
...

Could we hash that 👆instead?

@bungle
Copy link
Member Author

bungle commented Sep 23, 2020

We could only hash the rockspec dependencies, but not when adding a new file to Kong (or when adding a blank line to the rockspec).

resty -e "setmetatable(_G,nil); assert(loadfile('kong-2.1.3-0.rockspec'))(); table.sort(dependencies); for _,d in ipairs(dependencies) do print(d) end"

Output looks like:

inspect == 3.1.1
kong-lapis == 1.8.1.2
kong-plugin-acme ~> 0.2
...

Could we hash that 👆instead?

Or just cat kong-2.1.4-0.rockspec | awk '/dependencies/,/}/ { print }'

@bungle
Copy link
Member Author

bungle commented Sep 23, 2020

Something like:

DEPS_HASH=$({ cat .ci/setup_env.sh .travis.yml .requirements Makefile; cat kong-*.rockspec | awk '/dependencies/,/}/'; } | md5sum | awk '{ print $1 }')

### Summary

If it is not included in cache hash calculation, dependencies that are
updated in .rockspec are leaked to other tests / builds.
@bungle
Copy link
Member Author

bungle commented Sep 23, 2020

Something like:

DEPS_HASH=$({ cat .ci/setup_env.sh .travis.yml .requirements Makefile; cat kong-*.rockspec | awk '/dependencies/,/}/'; } | md5sum | awk '{ print $1 }')

I updated the PR with this change. Btw. we are also soon dropping travis... so... just to get things green until that.

@bungle
Copy link
Member Author

bungle commented Sep 24, 2020

I'll close this, let's just merge this to next:
#6383

@bungle bungle closed this Sep 24, 2020
@bungle bungle deleted the chore/tests-cache-rockspec branch September 24, 2020 08:36
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