Skip to content

GitHub actions for newsite branch - #46

Merged
Shekharrajak merged 21 commits into
newsitefrom
githubActions
Oct 6, 2019
Merged

Shekharrajak merged 21 commits into
newsitefrom
githubActions

Conversation

@Shekharrajak

@Shekharrajak Shekharrajak commented Oct 2, 2019 •

Copy link
Copy Markdown
Member
  • Github action : deploy-action created.
  • Github workflow "deploy" job will trigger the deploy-action
  • The pattern of the URL is : pr-<pr_number>-numpy.org-newsite.surge.sh

So for this PR it is : http://pr-46-numpy.org-newsite.surge.sh

Advantages:

  • Now reviewing the UI changes easy because no need to clone the branch and check the UI locally. It can be seen in the deployed URL.
  • PR author doesn't have to create GIF or attach screenshot for the UI changes. Dynamic changes can be seen easily in the URL.
  • Every PR will be having the deployed URL so we can see the transformation of the website easily.

@Shekharrajak

Copy link
Copy Markdown
Member Author

Tried to get PR number using : export PR_NUMBER="$(echo ${{REF}} | cut -d'/' -f3)"

and IFS='/' read -r -a array <<< "$REF"

Comment thread .github/workflows/main.yml Outdated
- ubuntu-latest
- windows-latest
steps:
- uses: actions/checkout@master

Copy link
Copy Markdown
Contributor

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.

@Shekharrajak Shekharrajak Oct 3, 2019 •

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

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

@WarrenWeckesser WarrenWeckesser Oct 5, 2019 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

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 masterbranch of https://github.com/actions/checkout

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here.

Comment thread Dockerfile
@@ -0,0 +1,16 @@
FROM node:10-alpine

Copy link
Copy Markdown
Contributor

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 :)

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info.

@joelachance

Copy link
Copy Markdown
Contributor

Looks good so far!

@Shekharrajak

Copy link
Copy Markdown
Member Author

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.

@Shekharrajak
Shekharrajak force-pushed the githubActions branch 2 times, most recently from eebf090 to 4553541 Compare October 3, 2019 15:28
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
@Shekharrajak

Shekharrajak commented Oct 3, 2019 •

Copy link
Copy Markdown
Member Author

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 Shekharrajak left a comment

Copy link
Copy Markdown
Member Author

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.

Comment thread .github/workflows/main.yml Outdated
env:
REF: ${{ github.ref}}
run: |
echo Checking github env variables:

Copy link
Copy Markdown
Member Author

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)

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member Author

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.

Comment thread .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 }}"

Copy link
Copy Markdown
Member Author

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.

@Shekharrajak

Copy link
Copy Markdown
Member Author

@joelachance @rgommers please review. If it is looking good then we can merge this branch to newsite for now.

@rgommers

rgommers commented Oct 5, 2019

Copy link
Copy Markdown
Member

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?

@Shekharrajak

Copy link
Copy Markdown
Member Author

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 SURGE_LOGIN and SURGE_TOKEN as environment variable. In github action we can add environment variables secretly in settings->secret tab. I have added my surge token and login mail ID. It can be changed in the future and it will not affect anything.

@joelachance

Copy link
Copy Markdown
Contributor

Looks good! Feel free to merge.

@Shekharrajak
Shekharrajak merged commit 84048be into newsite Oct 6, 2019
@rgommers
rgommers deleted the githubActions branch October 17, 2019 11:55
This was referenced Oct 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants