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

Upgrade to Python3 #3357

Merged
merged 4 commits into from
Feb 20, 2020
Merged

Upgrade to Python3 #3357

merged 4 commits into from
Feb 20, 2020

Conversation

simitt
Copy link
Contributor

@simitt simitt commented Feb 18, 2020

Motivation/summary

This PR upgrades APM Server to use Python 3. After elastic/beats#14798 has landed upgrading the beats framework changes python dependencies e.g. in the Makefile from python2 to python3.
In #3001 we agreed on not spending time on upgrading scripts and the system tests to python3. Syncing the latest beats framework showed that we would need to completely get rid of Makefile dependencies to avoid python version conflicts. The other option would have been to not upgrade the tests, but still upgrade the tooling, which would require handling two python versions.

Therefore I decided to look into what's necessary to actually upgrade the python version, as changing the Makefile and ensuring everything works as expected also costs time. Turns out that only a few adoptions in the system tests were necessary (took me less than 2 hours to upgrade).

So this PR contains

  • changes to the make update-beats script to sync the test files
  • updates to the latest beats framework
  • upgrading system tests to python3: main changes were related to testing TLS configurations, please have a closer look at that when reviewing
  • upgrading APM Server to python3

Checklist

- [ ] I have signed the Contributor License Agreement.

  • My code follows the style guidelines of this project (run make check-full for static code checks and linting)
  • I have rebased my changes on top of the latest master branch
  • I have made corresponding changes to the documentation
    - [ ] I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

- [ ] I have updated CHANGELOG.asciidoc

How to test these changes

CI passing should test a huge part of this. One-off scripts need to be tested manually.

Related issues

#3001

@Bamieh
Copy link
Member

Bamieh commented Feb 19, 2020

Codecov Report

Merging #3357 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3357   +/-   ##
=======================================
  Coverage   79.36%   79.36%           
=======================================
  Files         108      108           
  Lines        5698     5698           
=======================================
  Hits         4522     4522           
  Misses       1176     1176           

@simitt simitt marked this pull request as ready for review February 19, 2020 17:06
@simitt
Copy link
Contributor Author

simitt commented Feb 19, 2020

I did some manual testing for the tooling and as far as I could see everything works as expected.

@simitt simitt changed the title Update to Python3 Upgrade to Python3 Feb 19, 2020
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Looks good, thank you. Only thing I'm not too sure about is the Windows CI bit. It's fine, but are we installing in the right place? How was Python being installed before?

Also, there appears to be an issue with integration tests, it's complaining about python3-venv.

tests/system/apmserver.py Outdated Show resolved Hide resolved
@simitt
Copy link
Contributor Author

simitt commented Feb 20, 2020

Only thing I'm not too sure about is the Windows CI bit. It's fine, but are we installing in the right place? How was Python being installed before?

I went along with the according changes in beats, mainly because I wouldn't know how else to solve this, and didn't think this is problematic.

The failing integration test setup is concerning, thanks for pointing that out, will look into it.

@simitt
Copy link
Contributor Author

simitt commented Feb 20, 2020

Required changes for APM Integration Tests are addressed in elastic/apm-integration-testing#766.

@simitt
Copy link
Contributor Author

simitt commented Feb 20, 2020

We usually don't git rebase on PRs, but since it makes sense to keep multiple commits here and they should be squashed I am rebasing this one.

Update beats script to sync python system test files again.
* upgrade python code
* upgrade windows test setup to python3
add python-env to make targets
@simitt simitt merged commit c45982b into elastic:master Feb 20, 2020
@simitt simitt deleted the python3 branch February 21, 2020 13:23
v1v added a commit to v1v/apm-server that referenced this pull request Mar 5, 2020
v1v added a commit that referenced this pull request Mar 6, 2020
v1v added a commit to v1v/apm-server that referenced this pull request Mar 6, 2020
v1v added a commit to v1v/apm-server that referenced this pull request Mar 9, 2020
v1v added a commit that referenced this pull request Mar 10, 2020
* ci(jenkins): backport commits from long time ago (#3438)
* ci(jenkins): avoid builds in the master worker (#2599)
* ci(jenkins): build and test stages are now merged. (#2614)
* ci(jenkins): rename APM ITs to avoid any misleading (#2629)
* ci(jenkins): revert none agent (#2645)
* ci(jenkins): reuse top level agent (#2774)
* ci(jenkins): update argument (#2904)
* [apm-ci] When asciidoc speedup skip some stages (#2891)
* ci(jenkins): change log rotation, use cached repo (#2970)
* ci(jenkins): use git reference repo (#2971)
* feat: grab docker container logs after run tests (#2948)
* ci(jenkins): shallow cloning doesn't work with isGitRegionMatch (#2998)
* ci(jenkins): cancel previous running builds (#2983)
* feat: enable DIAGNOSTIC_INTERVAL env var (#3032)
* ci(jenkins): when regexp comparator (#2644)
* fix: trigger on _beats changes (#3189)
* ci(jenkins): retry build docker image up to 3 times with some sleep
* Simplify build scripts (#3371)
* ci(jenkins): apm-its-downstream params has changed (#3406)
* fix: build inside a Golang Docker container (#2375)
* ci: move benchmark stage to run in a tool Docker container (#2391)
* add package install test, add labels to script steps (#2268)
* [apm-ci] suppress the NativeCommandError in windows builds (#2890)
* ci(jenkins): fix issues with python3 in branch 6.8 (#3428)
* ci(jenkins): apm-its are not required in the CI Pipeline
* Use python3
* ci(jenkins): remove leftovers from the APM-ITS
* fix: build inside a Golang Docker container (#2375)
* fix: use fixed dependencies on golang Docker container (#2410)
* ci(jenkins): build CI Docker images (#2498)
* partial cherry-pick from #3357

Co-authored-by: Ivan Fernandez Calvo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants