-
Notifications
You must be signed in to change notification settings - Fork 748
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
release.yml run build before creating new tag #3032
Conversation
.github/workflows/release.yml
Outdated
- name: Checkout | ||
uses: actions/checkout@master | ||
with: | ||
fetch-depth: 0 | ||
- name: Build | ||
uses: actions/checkout@master | ||
uses: skx/github-action-build@master | ||
with: | ||
builder: make |
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.
few points here,
- The two steps mentioned above checkouts master branch twice. Instead, we can checkout the branch once and then proceeding to the build step.
- Build command will run successful even if certain unit tests fail. Should also execute the
validate.sh
script - Also should add
if
condition to check ifhasWritePermission
is true
build-master:
name: Build master
needs: check-permission
if: contains(needs.check-permission.outputs.hasWritePermission, 'true')
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@master
with:
fetch-depth: 0
repository: ${{ github.repository }}
ref: master
- name: Validate and build
run: |
./validate.sh
make build
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.
Thanks for the reivew. Addressed your comments in 9477772
The two steps mentioned above checkouts master branch twice. Instead, we can checkout the branch once and then proceeding to the build step.
Copy/paste error. Corrected
Build command will run successful even if certain unit tests fail. Should also execute the
validate.sh
script
Good point. Added. I ran the make
command before validate
though
Also should add
if
condition to check ifhasWritePermission
is true
Added
.github/workflows/release.yml
Outdated
go build . | ||
./validate.sh | ||
go build . |
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.
👍🏼
.github/workflows/release.yml
Outdated
- name: Build and validate | ||
run: | | ||
./validate.sh | ||
go build . |
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 am not sure we need the go build .
I don't think it is possible for it not to compile, but still manage to pass all the unit tests,
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.
Agree go build .
command will execute successfully only if all tests pass. Can update script to only run validate.sh
- name: Validate
run: ./validate.sh
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.
Good catch. Removed
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.
Looks good other than the one quibble.
bdd780f
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
Github actions
release.yml
is tagging before verifying whether or not the code compiles sucessfully. This led to tags0.268.0
and0.269.0
to both point to the same commit when the former's release failed. This pull request makesrelease.yml
pull from currentmaster
branch and perform a build before moving on to create a new tag.