-
-
Notifications
You must be signed in to change notification settings - Fork 119
GitHub actions for newsite branch #46
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
Conversation
Tried to get PR number using : and |
.github/workflows/main.yml
Outdated
- ubuntu-latest | ||
- windows-latest | ||
steps: | ||
- uses: actions/checkout@master |
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.
do we want to specify the newsite
branch for this? Obviously it will need to be master when we merge all the 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.
Yes, that has to be changed when we merge newsite branch to master.
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.
+1 for changing it to actions/checkout@newsite
now
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.
As far as I understand, checkout action need the tag of the repository not the branch. So it is basically action/checkout@<tags>
.I tried with newsite
and got this error in log: https://github.com/numpy/numpy.org/pull/46/checks?check_run_id=249019844
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.
My understanding is that the @
symbol used in an action
specifies the version of the action: https://help.github.com/en/articles/workflow-syntax-for-github-actions#jobsjob_idstepsuses
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 @WarrenWeckesser for the link. I am wondering why it is working with this@master
and the base branch for the PR is newsite
. Here I see that we can use specific branch using
- uses: actions/checkout@master
with:
ref: some-branch
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'm still figuring this out, so all I can say is that uses: actions/checkout@master
means you are, in effect, using the development version of the checkout
action, i.e. the master
branch of https://github.com/actions/checkout
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! It looks like you are correct. Since it is in this format {owner}/{repo}/{path}@{ref}
. and using with
option we are customizing the branch, sha, release version.
So I think basically it is using this PR branch and latest commit to run the action specified.
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 looks like the default branch (or SHA) that is checked out is explained in https://help.github.com/en/articles/events-that-trigger-workflows#webhook-events. In particular, see the GITHUB_SHA
column of the tables for pull_request
and push
events.
Also, README.md for the checkout
action says
By default, the branch or tag ref that triggered the workflow will be checked out. If you wish to check out a different branch, specify that using
with.ref
python-version: | ||
- 3.7 | ||
steps: | ||
- uses: actions/checkout@master |
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.
Same question here.
@@ -0,0 +1,16 @@ | |||
FROM node:10-alpine |
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.
Do we need node alpine? We might be able to use another image (something smaller, if it exists), although it probably doesn't really matter :)
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.
node docker image is used because later in deploy script it is installing the surge package.
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 maybe doesn't matter here, but Alpine is one of the most problematic distributions for Python packages, because it's musl rather than glibc based so manylinux
wheels don't work if they contain compiled code. Just FYI
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 info.
Looks good so far! |
Currently, I am getting issue in assigning variable values and in setting env variable inside of the docker container after extracting the PR number from the $github.ref env variable. I will clean the commits. |
eebf090
to
4553541
Compare
main.yml minor change main.yml minor change
minor change minor change
minor change
minor change in main.yml deploy.sh set the output name minor change - dockerfile to get var value minor change - dockerfile to get var value
4cd257f
to
c3059ba
Compare
d9d77ce
to
13dd632
Compare
Finally more than 50 try, it worked and it is able to extract the PR number from the github.ref variable inside the docker container (while running the bin/sh script). It is getting deployed in http://pr-46-numpy.org-newsite.surge.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.
We can remove the test job or improve it to make it work similar to travis test job.
.github/workflows/main.yml
Outdated
env: | ||
REF: ${{ github.ref}} | ||
run: | | ||
echo Checking github env variables: |
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.
This has to be removed and proper test should run (similar to the travis CI/CD)
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 don't really have tests. Maybe it can be checked that the Hugo build finishes without errors or warnings?
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 have removed this job for now. We can add jobs later as per the requirements.
.github/workflows/main.yml
Outdated
SURGE_LOGIN: ${{ secrets.SURGE_LOGIN}} | ||
REF: ${{ github.ref}} | ||
- name: Get the output time | ||
run: echo "The output was ${{ steps.deploy.outputs.deployed-domain }}" |
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.
This can be removed later. This is just to check the deployed-domain URL.
@joelachance @rgommers please review. If it is looking good then we can merge this branch to newsite for now. |
This looks pretty clean to me, happy to see it merged. One question about the Surge setup: is this PR all that is needed, or do we need to have some account with access credentials that need storing somewhere? |
Thanks for the review. To deploy the static contents we need to have |
Looks good! Feel free to merge. |
pr-<pr_number>-numpy.org-newsite.surge.sh
So for this PR it is : http://pr-46-numpy.org-newsite.surge.sh
Advantages: