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

[MRG] Add basic GitHub Actions workflow for testing #1209

Merged
merged 33 commits into from
Nov 13, 2020

Conversation

betatim
Copy link
Member

@betatim betatim commented Nov 8, 2020

This PR will reimplement our Travis testing setup on GitHub Actions. The goal is to retire Travis as quickly as possible and use only GitHub Actions.

Heavily based on https://github.com/manics/zero-to-jupyterhub-k8s/blob/gh-workflow/.github/workflows/test.yml

@betatim betatim force-pushed the add-gh-actions branch 11 times, most recently from 6983cd6 to 813949a Compare November 8, 2020 11:41
Changes to allow us to determine the JupyterHub's URL at runtime both
when we use minikube and when we use k3s.
@consideRatio
Copy link
Member

Here is an extract from the recent pebble-helm-chart migration I attempted. I wanted to highlight the on failure/on success steps. I suggest we add a step running the full_namespace_report function on failure.

      - name: Run tests
        run: |
          helm lint pebble --values ci/ci-values.yaml
          helm lint pebble --strict --values ci/ci-values.yaml
          helm template pebble ./pebble --values ci/ci-values.yaml --validate
          helm install pebble ./pebble --values ci/ci-values.yaml --wait --timeout 60s
          helm test pebble
      - name: Emit report on success
        if: success()
        run: |
          kubectl get all --all-namespaces
      - name: Emit report on failure
        if: failure()
        run: |
          kubectl get all --all-namespaces
          kubectl logs pebble-test --all-containers --prefix
          kubectl logs pebble-coredns-test --all-containers --prefix
          kubectl logs deploy/pebble-coredns --all-containers --prefix

@@ -7,7 +9,8 @@ service:

config:
BinderHub:
hub_url: http://192.168.99.100:30902
# don't set `hub_url` here, it needs to be set from Python code as we need
# to determine the IP at runtime.
Copy link
Member

@consideRatio consideRatio Nov 8, 2020

Choose a reason for hiding this comment

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

I'm not 100%, but I think we may want to set hub_url_local: http://hub:8001

Since this is a configuration for binderhub the Helm chart, then I think that's fine, because then we can set hub_url to be the public url no matter if that URL is something that the binderhub pod within the k8s cluster can reach or not, because the binderhub pod will always rely on the hub_url_local over the hub_url (public) if it is set.

@consideRatio
Copy link
Member

@betatim I added two commits to make the chart publishing work with GitHub actions, but there is one part missing still about DOCKER_USERNAME and DOCKER_PASSWORD. I hope that Min can help us with this and I have already pinged him to do this for z2jh and binderhub. See the concrete request in gitter if you happen to have the password for the dockerhub user jupyterhubbot

@betatim
Copy link
Member Author

betatim commented Nov 8, 2020

Ah, sorry I didn't see your commits before I force pushed. I'll integrate those things again later.

@consideRatio
Copy link
Member

@betatim no worries, I created a PR for it as i still had them locally: betatim#2

@betatim
Copy link
Member Author

betatim commented Nov 8, 2020

edit: seems like I need to get better at parsing regex in my head. Ignore comment below.


A bit puzzled why the tests fail. Or to put it differently: I am confused why this test ever passed. This is because when I run the code that the test exercises locally (with the same input) it raises an exception much earlier than where things go wrong in the test. Reading the code I expect what I see when I run the code in ipython. The output from the test:

_____________________________ test_hydroshare_doi ______________________________

    async def test_hydroshare_doi():
        spec = '10.4211/hs.b8f6eae9d89241cf8b5904033460af61'
    
        provider = HydroshareProvider(spec=spec)
    
>       ref = await provider.get_resolved_ref()

binderhub/tests/test_repoproviders.py:134: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/opt/hostedtoolcache/Python/3.8.6/x64/lib/python3.8/site-packages/tornado/gen.py:742: in run
    yielded = self.gen.throw(*exc_info)  # type: ignore
binderhub/repoproviders.py:374: in get_resolved_ref
    r = yield client.fetch(req)
/opt/hostedtoolcache/Python/3.8.6/x64/lib/python3.8/site-packages/tornado/gen.py:735: in run
    value = future.result()
binderhub/tests/utils.py:79: in fetch
    self._record_response(url_key, response)

The code in question is: https://github.com/jupyterhub/binderhub/blob/master/binderhub/repoproviders.py#L371-L374 which I would expect to fail when calling self._parse_resource_id(self.spec) when spec = "10.4211/hs.b8f6eae9d89241cf8b5904033460af61".

So somewhere that DOI is being resolved that I can't spot :-/

@betatim
Copy link
Member Author

betatim commented Nov 9, 2020

#1209 (comment) was a flakey test maybe or a short downtime of Hydroshare? Locally I got the same error because I misspelt the argument specc= not spec=, I think.

@betatim
Copy link
Member Author

betatim commented Nov 13, 2020

I've incorporated Erik's PR with changes for publishing the chart. I've not tested them/know too little about that to know if they will work or not though. Would be good for someone to take a look at them.

@betatim betatim changed the title [WIP] Add basic GitHub Actions workflow for testing [MRG] Add basic GitHub Actions workflow for testing Nov 13, 2020
@betatim
Copy link
Member Author

betatim commented Nov 13, 2020

I think this is ready for reviewing. Maybe @minrk can take a look over.

I think there are a few more things we could do (like fix the code scanning) but I'd address those in a new PR because there are PRs with actual content that are waiting to have working CI again. The most important thing to check once we have this merged is if the publishing of the chart works (so that henchbot and mybinder.org deploys can resume).

.github/workflows/test.yml Outdated Show resolved Hide resolved
@consideRatio
Copy link
Member

Wieee, @betatim do you also get that nice feeling? ;D

image

@betatim
Copy link
Member Author

betatim commented Nov 13, 2020

I get the feeling that we had to invest a lot of hours to get back to where we were before :D

@consideRatio consideRatio merged commit 42f8538 into jupyterhub:master Nov 13, 2020
@consideRatio
Copy link
Member

Wieee! With my own LGTM and the [MRG] title update and all comments resolved etc its time for a merge I think!

Let's see if things work as they are intended!

@consideRatio
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Under the hood improvements and fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants