Repository navigation
GitHub actions for newsite branch - #46
Conversation
|
Tried to get PR number using : and |
| - ubuntu-latest | ||
| - windows-latest | ||
| steps: | ||
| - uses: actions/checkout@master |
There was a problem hiding this comment.
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.
Yes, that has to be changed when we merge newsite branch to master.
There was a problem hiding this comment.
+1 for changing it to actions/checkout@newsite now
There was a problem hiding this comment.
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.
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.
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.
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 masterbranch of https://github.com/actions/checkout
There was a problem hiding this comment.
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.
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 |
| @@ -0,0 +1,16 @@ | |||
| FROM node:10-alpine | |||
There was a problem hiding this comment.
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.
node docker image is used because later in deploy script it is installing the surge package.
There was a problem hiding this comment.
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.
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 |
Shekharrajak
left a comment
There was a problem hiding this comment.
We can remove the test job or improve it to make it work similar to travis test job.
| env: | ||
| REF: ${{ github.ref}} | ||
| run: | | ||
| echo Checking github env variables: |
There was a problem hiding this comment.
This has to be removed and proper test should run (similar to the travis CI/CD)
There was a problem hiding this comment.
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.
I have removed this job for now. We can add jobs later as per the requirements.
| 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.
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.shSo for this PR it is : http://pr-46-numpy.org-newsite.surge.sh
Advantages: