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

Fix unusable error when migrations are broken. #2821

Closed
wants to merge 2 commits into from

Conversation

ikogan
Copy link
Contributor

@ikogan ikogan commented Aug 18, 2017

Summary

When a migration is defined without a name, Kong fails to start with a fairly useless error message:

/usr/local/share/lua/5.1/kong/cmd/start.lua:62: /usr/local/share/lua/5.1/kong/cmd/utils/log.lua:49: bad argument #6 to 'format' (no value)
stack traceback:
        [C]: in function 'error'
        /usr/local/share/lua/5.1/kong/cmd/start.lua:62: in function 'cmd_exec'
        /usr/local/share/lua/5.1/kong/cmd/init.lua:88: in function </usr/local/share/lua/5.1/kong/cmd/init.lua:88>
        [C]: in function 'xpcall'
        /usr/local/share/lua/5.1/kong/cmd/init.lua:88: in function </usr/local/share/lua/5.1/kong/cmd/init.lua:45>
        /usr/local/bin/kong:7: in function 'file_gen'
        init_worker_by_lua:38: in function <init_worker_by_lua:36>
        [C]: in function 'xpcall'
        init_worker_by_lua:45: in function <init_worker_by_lua:43>

This is because migration.name, being nil, when passed to the log function causes the format string to fail to resolve. This simply uses "(no name)" in place of the name.

@thibaultcha
Copy link
Member

thibaultcha commented Aug 18, 2017

Hi,

Names are mandatory for migrations, otherwise Kong has now way to decide whether a migration has been run already or not. Granted, Kong could catch that a migration has no name and should yield an appropriate error if so.

Will you contribute such a patch? 😁

@ikogan
Copy link
Contributor Author

ikogan commented Aug 18, 2017

This actually does end up yielding an error about the migration having never run. I can likely change it to be a more explicit error.

@thibaultcha
Copy link
Member

Yes; I meant that Kong should fail earlier in such a case. This log message that you patched is-user friendly, and helps the user identify which migrations haven't run yet.

However, a migration that has no name is a developer error, and should be caught earlier (when the factory loads said migrations from the core/plugins) so that a developer can spot their mistake and fix it before the plugin is distributed. Something more or less like this maybe:

for i, migration in ipairs(migrations) do
  if type(migration.name) ~= "string" then -- cal also check that names are unique (another requirement)
    error("migration ", i, " for DAO ", dao_name, " must have a unique name")
  end
end

@thibaultcha thibaultcha added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Aug 19, 2017
@ikogan
Copy link
Contributor Author

ikogan commented Aug 21, 2017

I've implemented the two checks. I noticed some of the CI tests were failing before but I don't see how they would have had to do with my changes.

@thibaultcha thibaultcha added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Aug 28, 2017
@thibaultcha
Copy link
Member

Thanks, I manually committed this with some refactoring to avoid some back and forth. Thanks again!

@thibaultcha thibaultcha closed this Sep 1, 2017
@ikogan ikogan deleted the ohio-migration-error-fix branch September 2, 2017 19:41
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.

2 participants