-
Notifications
You must be signed in to change notification settings - Fork 66
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
Automatic packaging and CI test updates #386
Conversation
8ed83bb
to
0a127a4
Compare
aba4143
to
d09450d
Compare
fe57438
to
b295172
Compare
Since I'm not familiar with Github Actions I had a quick question about the following piece of code (used for instance in
Do we allow pushing commits directly to those branches (if so why/when)? Or are they "push protected" in a way? Maybe is this CI rule used for tagging the release commit? The |
Hi @AdrienFontaine-PASQAL , thanks for taking the time to look into this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as my expertise go on Github Actions this sounds good to me.
Great work, and thanks for the nice and clear documentation! I only have a few tiny comments. |
- name: Type check with mypy | ||
run: mypy | ||
test: | ||
if: github.event_name != 'push' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Why do we not run test on push
events ? Not super familiar with all the event types in github actions, so maybe these test are triggered through a merge_request
event or something which would be redundant with the push
event
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just because I wrote a more complete test workflow to run only on push
to the protected branches, so these would be redundant.
run: | | ||
version="$(head -1 VERSION.txt)" | ||
python -c "import pulser; assert pulser.__version__ == '$version'" | ||
grep -e pytest dev_requirements.txt | sed 's/ //g' | xargs pip install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: As per my other comment, having specific extras_require
would have the extra benefit to avoid duplicating this grep
magic several times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True indeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are mostly questions about your overall flow (therefore hopefully complementary to Adrien and Matthieu's review ) :)
branches: | ||
- master | ||
- develop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean that these tests would not run on feature branches before merging to develop
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. I figured they are a bit excessive to be ran on every update to a PR. There are still tests in the ci.yml
that should catch everything, these tests are run just to really ensure we didn't miss anything related to a specifc OS or python version.
On the rare case something fails, we can always follow up with a fix.
requirements
anddev_requirements
build
workflow to install only the necessary packages for each jobtest
workflow to run the unit tests across different OSs and python versions (triggered only on push events tomaster
anddevelop
)master
anddevelop
Closes #363 .