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.

Sign up for GitHub

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

Jump to bottom

Add IT tests to test installation according to docs #1193

Merged
merged 6 commits into from
Feb 23, 2021
Merged

Add IT tests to test installation according to docs #1193

merged 6 commits into from
Feb 23, 2021

Conversation

dliappis
Copy link
Contributor

As a followup to #1185 this commit adds a few IT tests to validate the
Rally installation steps in the docs.

@dliappis
Add IT tests to test installation according to docs
416cf6b 
As a followup to elastic#1185 this commit adds a few IT tests to validate the
Rally installation steps in the docs.

We also remove hardcoded Python versions from the installation section
of docs instead relying on a checked-in variable to ease maintainance
in the future.
@dliappis dliappis added enhancement Improves the status quo :Packaging Installation issues or packaging problems labels Feb 22, 2021
@dliappis dliappis added this to the 2.1.0 milestone Feb 22, 2021
@dliappis dliappis self-assigned this Feb 22, 2021
@danielmitterdorfer
Copy link
Member

The CI build has failed with:


13:37:54 pyenv: version `3.8.7' is not installed (set by /var/lib/jenkins/workspace/elastic+rally+pull-request/.python-version)
13:37:54 Created python3 venv under /var/lib/jenkins/workspace/elastic+rally+pull-request/.venv.
13:37:54 . .venv/bin/activate; python3 -m pip install --upgrade pip setuptools wheel
13:37:54 /bin/bash: .venv/bin/activate: No such file or directory
13:37:54 pyenv: version `3.8.7' is not installed (set by /var/lib/jenkins/workspace/elastic+rally+pull-request/.python-version)
13:37:54 Makefile:65: recipe for target 'install-user' failed

I think we need to add:

pyenv install --skip-existing $(PYVER4DOCS)

in the prereq target of the Makefile.

@dliappis
Copy link
Contributor Author

Hm

The CI build has failed with:

Hm I think something else is wrong as actually I don't have 3.8.7 in my local pyenv's and still it's working fine. Will investigate

$ pyenv versions
  system
  2.7.18
  2.7.18/envs/ve
* 3.8.0 (set by /home/dl/source/elastic/rally/.python-version)
  3.8.5
* 3.9.0 (set by /home/dl/source/elastic/rally/.python-version)
  ve

@dliappis
Copy link
Contributor Author

I pushed e008198 so that standard make prereq doesn't install a Python version that isn't needed locally (we use a Docker image for the specific IT test).

@dliappis
Copy link
Contributor Author

@danielmitterdorfer I refactored as per our offline discussion in 3374548 to not introduce a new DOCS CI variable but instead rely on the existing ones. While at it, bumped the patch versions. Feel free to review after CI gets green.

danielmitterdorfer
danielmitterdorfer reviewed Feb 23, 2021
View reviewed changes
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Thanks! I left a couple of suggestions.

it/__init__.py Outdated
return os.system(command_line)


def command_in_docker(command_line, python_version="3.8.7"):
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should not have a default argument here so we always need to specify a version explicitly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.

it/installation_test.py Outdated
import it

_CI_VARS = ".ci/variables.json"
_FALLBACK_MIN_PY_VER = "3.8.7"
Copy link
Member

Choose a reason for hiding this comment

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

Should this also be 3.8.8? Should we read this from _CI_VARS instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, this is what we did before this refactoring in 3374548#diff-c0d0ee48d5c24db80722a8fef4cb274fea04db4fb2cd8c4147697758de4e9192L31-L38, will bring this back.

Makefile Outdated
@@ -21,8 +21,8 @@ PYENV_REGEX = .pyenv/shims
PY_BIN = python3
# https://github.com/pypa/pip/issues/5599
PIP_WRAPPER = $(PY_BIN) -m pip
PY38 = $(shell jq '.python_versions.PY38' .ci/variables.json)
PY39 = $(shell jq '.python_versions.PY39' .ci/variables.json)
export MIN_PY_VER = $(shell jq -r '.python_versions.PY38' .ci/variables.json)
Copy link
Member

Choose a reason for hiding this comment

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

Can we just alias this? i.e. something like:

export PY38 = $(shell jq -r '.python_versions.PY38' .ci/variables.json)
export MIN_PY_VER = $(PY38)

Copy link
Contributor Author

@dliappis dliappis Feb 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We can; what is the purpose if we won't be using PY38 though?

Copy link
Member

Choose a reason for hiding this comment

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

The idea behind this is that we derive the version variables always consistently. We should use the variable PY38 though when installing Python versions in make install. MIN_PY_VER can be used anywhere where we don't need to refer to a specific version but we want to convey the intent that we're looking for the minimum supported one.

@dliappis
Copy link
Contributor Author

CI failed specifically with py39 (test_tar_distributions[es-it]) and since nothing has changed it, I will just wait for the new CI results triggered by ae8ae56 and consider the failure transient.

@dliappis
Copy link
Contributor Author

Hm another CI failure this time with

************* Module tests.driver.scheduler_test
14:24:42 tests/driver/scheduler_test.py:95:28: E1102: s.next is not callable (not-callable)

but it's not reproducible locally with exactly the same pylint version.

@elasticmachine please test this

@dliappis
Copy link
Contributor Author

@elasticmachine test this please

@dliappis
Copy link
Contributor Author

@danielmitterdorfer CI is green (at last!) could you take another pass when you have time?

danielmitterdorfer
danielmitterdorfer approved these changes Feb 23, 2021
View reviewed changes
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Great that we have a test for this now. Thanks for iterating. LGTM

@dliappis dliappis merged commit 3875968 into elastic:master Feb 23, 2021
@dliappis dliappis deleted the installation_it_tests branch February 23, 2021 16:11
@dliappis
Copy link
Contributor Author

@danielmitterdorfer thanks a lot for the review!

dliappis added a commit to dliappis/rally that referenced this pull request Mar 23, 2021
@dliappis
Fix auto build of docs
bed9096 
PR elastic#1193 introduced a bug with readthedocs.org in relying on presenting
the MIN_PY_VER env var via the Makefile. However the Makefile isn't
invoked during automated build of readthedocs.

This commit fixes automatic builds of our docs again.
@dliappis dliappis mentioned this pull request Mar 23, 2021
Merged
dliappis added a commit that referenced this pull request Mar 26, 2021
@dliappis
Fix auto build of docs (#1217)
3b3acde 
PR #1193 introduced a bug with readthedocs.org in relying on presenting
the MIN_PY_VER env var via the Makefile. However the Makefile isn't
invoked during automated build of readthedocs.

This commit fixes automatic builds of our docs again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielmitterdorfer danielmitterdorfer danielmitterdorfer approved these changes

Assignees

@dliappis dliappis

Labels
enhancement Improves the status quo :Packaging Installation issues or packaging problems
Projects
None yet
Milestone
2.1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@dliappis @danielmitterdorfer