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: introduce a CLI interface and a Makefile. #66
Conversation
The CLI interface is not in use now.
// cmd.PersistentFlags().StringVar(&conf.SyslogServer, "syslog-server", "", "syslog server address") | ||
|
||
return cmd | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing test cases
@@ -7,5 +7,5 @@ sed -i -e "s%#syslogAddress#%`echo $SYSLOG_HOST`%g" ${pwd}/conf.json | |||
sed -i -e "s%#apisixBaseUrl#%`echo $APISIX_BASE_URL`%g" ${pwd}/conf.json | |||
|
|||
cd /root/ingress-controller | |||
exec ./ingress-controller | |||
exec ./ingress-controller ingress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we need the argument ingress
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We put all core logic about "ingress controller" in the sub command ingress, and we can extend this tool by adding others sub command, for instance, some debugging tools to fetch its internal states, or adding the service apis support by the "gateway" sub command. So using sub commands is more flexible.
It's not easy to add test cases now, since the config is so tricky, then program cannot run unless we modify the configuration file manually. I will add test cases after we refactoring the configuration module. |
I have created an issue #70 to track the test cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, add more E2E test in CI
The CLI interface is not in use now.