Reportengine CI #65
Reportengine CI #65
Conversation
.github/workflows/tests.yml
Outdated
| - name: Set up Python ${{ matrix.python-version }} | ||
| uses: actions/setup-python@v2 | ||
| with: | ||
| python-version: ${{ matrix.python-version }} | ||
|
|
||
| - name: Setup Miniconda | ||
| uses: conda-incubator/setup-miniconda@v3 | ||
| with: | ||
| activate-environment: test | ||
| use-mamba: true |
There was a problem hiding this comment.
Why do you need both of these?
There was a problem hiding this comment.
Hi @RoyStegeman, thanks for the comments.
The conda environment is needed because of pandoc. Does that answer your question?
There was a problem hiding this comment.
That I understood, but why do you need the python one?
There was a problem hiding this comment.
Right now the first step is needed to fix the python version.
But I guess we could do it directly in the miniconda env.
I can try to do so if you prefer it
There was a problem hiding this comment.
Yes it supports a python-version key, so I guess it makes sense to use that
.github/workflows/tests.yml
Outdated
| - name: Update Environment | ||
| shell: bash -l {0} | ||
| run: | | ||
| conda env update -n test -f environment.yml |
There was a problem hiding this comment.
Oh I thought I wrote a comment on this as well. Anyway, why do you not just do conda install pandoc followed by pip install . instead of declaring the dependencies in an evironment.yml?
There was a problem hiding this comment.
Hi Roy, sorry for the very late reply. I somehow forgot about this PR.
I think that having the environment.yml has the advantage of allowing the user to install reportengine and creating a conda env at the same time:
conda env create -f environment.yml
If we agree on keeping the env file, then why not using it directly in the CI?
There was a problem hiding this comment.
Also a long time ago for me...
Anyway, the dependencies are already defined in meta.yaml for installing with conda and pyproject.toml for python. In the test you already build the conda package, so you could create an env using the local package and run the test on that. That way you don't have to define the dependencies in yet another place.
Also conda install --only-deps exists, so people could use that to install deps instead of environment.yaml.
There was a problem hiding this comment.
Can you be a bit more specific please?
I am not sure the conda package is built in this test. I am only setting up miniconda.
|
As I was cleaning up my github notifications I came across this again, apologies about that. Looking at this now, I'm not so sure this PR is actually necessary. The conda-recipe/meta.yaml file already runs the tests upon building the package with conda-build. See here: reportengine/conda-recipe/meta.yaml Lines 28 to 34 in 02acde3 Did we miss this before, or is there another reason for this PR that I forgot about? |
run pytest on creation of pulll request