-
Notifications
You must be signed in to change notification settings - Fork 0
Update api-manager and go-api ref to latest version #56
base: main
Are you sure you want to change the base?
Update api-manager and go-api ref to latest version #56
Conversation
.github/workflows/test-build.yml
Outdated
@@ -33,7 +33,7 @@ jobs: | |||
with: | |||
go-version: '1.16.2' | |||
- name: Run go tidy | |||
run: make tidy && git diff --exit-code && test -z "$(git ls-files --exclude-standard --others | tee /dev/fd/2)" | |||
run: make tidy |
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.
What's the reason for dropping this test part?
I assume it's there to ensure make tidy
hasn't made any changes
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.
You're right.
I still want to test it with only the git diff --exit-code
part because according to the docs it will return 1 if there are differences and if that works it will simplify this step
@@ -56,7 +56,7 @@ EOF | |||
cat <<EOF > "${file}" | |||
name: e2e test ${major} | |||
|
|||
on: [push, pull_request] |
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 are we not performing e2e tests for PRs?
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.
If i'm right @nolancon in the old way it had executed two times, once commit had pushed and one pr had created. Now it runs only once.
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.
I guess, but if a PR came from a fork then the test would not run I think? 🤔 I know that we can't fork this repo because it's private, but presumably it will be public eventually. I often create PRs to our public repos from my own fork. Maybe I'm confusing things... it's not a big deal regardless 🤷♂️
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's exactly as @mhmxs said.
About the second concern, even if you fork a project you do it with the intention of making changes so when you do them and follow up with a commit+push the workflows will run.
In other words, what you described will indeed cause the workflows on our repo to not run but they will have run on the forked project.
IMO 99.99% of the contributions will be ours and thus this is not a concern. WDYT
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, that's fair enough. It probably makes more sense for us to be creating PRs that don't come from our own forked repos anyway.. just a habit I have from other OS projects I guess.
This will require testing which I will do on a follow up commit
`git diff --shortstat` will limit the output text avoiding spamming the action output with large diffs (makes it a lot more difficult to read) and the `--exit-code` option alone does what we want.
No description provided.