search

zulip / python-zulip-api

Python library for the Zulip API.
https://zulip.com/api/
Apache License 2.0
350 stars 356 forks source link

Setup CI using Github Actions #643

Closed LoopThrough-i-j closed 3 years ago

rht commented 3 years ago

@LoopThrough-i-j you should add a commit to remove .travis.yml at the end.

rht commented 3 years ago

@LoopThrough-i-j you haven't included the static analysis https://github.com/zulip/python-zulip-api/blob/master/.travis.yml#L9-L14.

rht commented 3 years ago

Your commit messages should end with a fullstop ('.').

rht commented 3 years ago

For other reviewers wanting to check the build: https://github.com/LoopThrough-i-j/python-zulip-api/actions.

LoopThrough-i-j commented 3 years ago

@LoopThrough-i-j you haven't included the static analysis https://github.com/zulip/python-zulip-api/blob/master/.travis.yml#L9-L14.

Thanks a lot for your review, will add the static analysis

Your commit messages should end with a fullstop ('.').

I am sorry for this, probably the lint for git commit is not set.

alexmv commented 3 years ago

Thanks for doing this, @LoopThrough-i-j! As I mentioned in the other PR, having confidence the the current state of the tests is really key, so not having it show up on PRs is really important to get fixed -- so thank you for tackling that. @rht's commented on more or less everything I can think of, so I don't have much to add. :)

rht commented 3 years ago

@LoopThrough-i-j nit: your Dropbox commit message is more concise if you just say: 'dropbox_share: Pin version of dropbox dependency.' and then in the secondary description of the commit message say that the version after 10.10.0 fails the test.

rht commented 3 years ago

@LoopThrough-i-j you should add a commit to remove .travis.yml at the end.

You forgot to react to this.

LoopThrough-i-j commented 3 years ago

@LoopThrough-i-j you should add a commit to remove .travis.yml at the end.

You forgot to react to this.

@rht I didn't remove the .travis.yml to keep it for reference. I will remove it from my final commit once everyone is fine with it.

rht commented 3 years ago

Typo in the Dropbox commit message: "verion".

rht commented 3 years ago

Also, you must use imperative statement, not "fixed". I think it is more relevant to say "Pin" than "Fix".

rht commented 3 years ago

You forgot full stop for the first commit again.

LoopThrough-i-j commented 3 years ago

You forgot full stop for the first commit again.

Yeah, I added it now. I think we don't have git lint set here. Can I work on that next? @rht

rht commented 3 years ago

I think we don't have git lint set here. Can I work on that next?

Sounds good. This is the issue on zulip/zulip, and this the commit that closed it.

rht commented 3 years ago

Another nit: in the commit message "pin" and "setup" should be capitalized. E.g. see https://github.com/zulip/zulip/commit/42dfd9860702ed4dc450023fbf22e7bbd88ae8de.

LoopThrough-i-j commented 3 years ago

Another nit: in the commit message "pin" and "setup" should be capitalized. E.g. see zulip/zulip@42dfd98.

@rht This is updated I hope everything looks good now. We can probably resolve the unresolved issue above and Keep the Jobs separate for separate reports.

I think we don't have git lint set here. Can I work on that next?

Sounds good. This is the issue on zulip/zulip, and this the commit that closed it.

Yes, I will create a new PR for Git Lint Setup.

timabbott commented 3 years ago

@LoopThrough-i-j I don't see CI as having run on this PR -- any idea why?

LoopThrough-i-j commented 3 years ago

@timabbott because it runs on push to master

on:
  push:
    branches:
    - master

and for all pull requests

  pull_request:

The branch I committed to in my fork is not master but named CI. I have tested it out thoroughly to make it work, and have got all the issues fixed.

timabbott commented 3 years ago

Merged the first commit, after copying the commit message into a comment, as e995e52896a9fa93678eedb777d6d63927913bb5.

Can you post a link to the CI run output with GitHub Actions?

LoopThrough-i-j commented 3 years ago

Merged the first commit, after copying the commit message into a comment, as e995e52.

Can you post a link to the CI run output with GitHub Actions?

This Actionswon't run till it is merged in the original repository. Because the Repo does not have the required build files to run it. I can run it on my fork by removing master from it. 

branches:
    - master

Hopefull I could explain it to you.

neiljp commented 3 years ago

@timabbott zulip/zulip has on [push, pull_request] in almost all workflows (except production). There is discussion above on this alternative, but I'd be interested to hear what you think since the choice in this PR is different to zulip/zulip.

rht commented 3 years ago

I can run it on my fork by removing master from it.

@LoopThrough-i-j alternatively you can make a PR to the master branch in your repo, so you don't have to modify any code.

rht commented 3 years ago

FTR @andersk restricted the push to only master (https://github.com/zulip/zulip-desktop/commit/a1bb6da4fb3ffd2007b147c184f64300711094f4) at zulip/zulip-desktop.

LoopThrough-i-j commented 3 years ago

@timabbott here is a sample build I triggered to test if everything still works, for clarity.

alexmv commented 3 years ago

Thank you for being persistent on this, and apologies for the delay. I've reformatted the yml with prettier and merged.

LoopThrough-i-j commented 3 years ago

Thank you for being persistent on this, and apologies for the delay. I've reformatted the yml with prettier and merged.

No problem thanks.