Skip to content
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

GitHub actions for newsite branch #46

Merged
merged 21 commits into from
Oct 6, 2019
Merged

GitHub actions for newsite branch #46

merged 21 commits into from
Oct 6, 2019

Conversation

Shekharrajak
Copy link
Member

@Shekharrajak Shekharrajak commented Oct 2, 2019

  • 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
Member Author

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

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

- ubuntu-latest
- windows-latest
steps:
- uses: actions/checkout@master
Copy link
Collaborator

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.

Copy link
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
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
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

Copy link
Member

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

Copy link
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
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
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
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
Collaborator

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
Copy link
Collaborator

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
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
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
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
Collaborator

Looks good so far!

@Shekharrajak
Copy link
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
Copy link
Member Author

Shekharrajak commented Oct 3, 2019

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

Copy link
Member Author

@Shekharrajak Shekharrajak left a 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.

env:
REF: ${{ github.ref}}
run: |
echo Checking github env variables:
Copy link
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
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
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.

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
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
Member Author

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

@rgommers
Copy link
Member

rgommers commented Oct 5, 2019

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
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
Collaborator

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.

None yet

4 participants