Skip to content

Conversation

@ElWPenn
Copy link

@ElWPenn ElWPenn commented Feb 19, 2020

What does this pull request do?

Adds additional tests for windows on different versions of python

Why is it important?

Resolves potential flakiness from Appveyor tests by deprecating them

Related issues

closes

@cla-checker-service
Copy link

Author of the following commits did not sign a Contributor Agreement:
99e68d9, a69ef7e

Please, read and sign the above mentioned agreement if you want to contribute to this project

@cla-checker-service
Copy link

Author of the following commits did not sign a Contributor Agreement:
99e68d9, a69ef7e, aa4b9a2, af93be4

Please, read and sign the above mentioned agreement if you want to contribute to this project

@beniwohli
Copy link
Contributor

beniwohli commented Feb 19, 2020

Great stuff! This is in no way required, but would it be possible to always install the newest version of a certain major.minor release train? That way, when e.g. a new 3.8 patch version comes out, we don't need to update the Jenkinsfile.

I did a quick search for defining version ranges with Chocolatey, and from what I can tell it doesn't seem to be supported. So the answer to this might very well be "not possible" :)

@kuisathaverat
Copy link
Contributor

I did a quick search for defining version ranges with Chocolatey, and from what I can tell it doesn't seem to be supported. So the answer to this might very well be "not possible" :)

It is more a not in an easy way, we can open a follow-up issue to investigate if install the edge version is not complicated.

@beniwohli
Copy link
Contributor

beniwohli commented Feb 19, 2020

@kuisathaverat OK, thanks for the update. If it isn't easily solvable, I don't think it's worth the needed effort. Hardcoding the exact version numbers in Jenkinsfile is fine for now.

@cla-checker-service
Copy link

Author of the following commits did not sign a Contributor Agreement:
99e68d9, a69ef7e, aa4b9a2, af93be4, c88e611

Please, read and sign the above mentioned agreement if you want to contribute to this project

@cla-checker-service
Copy link

Author of the following commits did not sign a Contributor Agreement:
99e68d9, a69ef7e, aa4b9a2, af93be4, c88e611, bd0af6e

Please, read and sign the above mentioned agreement if you want to contribute to this project

@cla-checker-service
Copy link

Author of the following commits did not sign a Contributor Agreement:
99e68d9, a69ef7e, aa4b9a2, af93be4, c88e611, bd0af6e, 45760a8

Please, read and sign the above mentioned agreement if you want to contribute to this project

@cla-checker-service
Copy link

Author of the following commits did not sign a Contributor Agreement:
99e68d9, a69ef7e, aa4b9a2, af93be4, c88e611, bd0af6e, 45760a8, fa8d3ab

Please, read and sign the above mentioned agreement if you want to contribute to this project

@v1v
Copy link
Member

v1v commented Feb 20, 2020

@beniwohli, @kuisathaverat and @ElWPenn , regarding installing the latest version for the given major.minor I did code something similar for the Node.js for the major version using chocolatey, see:

https://github.com/elastic/apm-agent-nodejs/pull/1393/files#diff-860c4fe077b91b560f207db63e491a6a

I guess something like the above script could be modified potentially to search for <major>.<minor>.

I managed to create a PR with the proposal, see #730

@cla-checker-service
Copy link

Author of the following commits did not sign a Contributor Agreement:
99e68d9, a69ef7e, aa4b9a2, af93be4, c88e611, bd0af6e, 45760a8, fa8d3ab, 0020034

Please, read and sign the above mentioned agreement if you want to contribute to this project

@cla-checker-service
Copy link

Author of the following commits did not sign a Contributor Agreement:
99e68d9, a69ef7e, aa4b9a2, af93be4, c88e611, bd0af6e, 45760a8, fa8d3ab, 0020034, 0c57059

Please, read and sign the above mentioned agreement if you want to contribute to this project

@beniwohli
Copy link
Contributor

@ElWPenn any chance you could set your git email to your elastic.co email to keep down the CLAchecker noise? Thanks :)

@cla-checker-service
Copy link

Author of the following commits did not sign a Contributor Agreement:
99e68d9, a69ef7e, aa4b9a2, af93be4, c88e611, bd0af6e, 45760a8, fa8d3ab, 0020034, 0c57059, 3e452ae

Please, read and sign the above mentioned agreement if you want to contribute to this project

@cla-checker-service
Copy link

cla-checker-service bot commented Feb 25, 2020

❌ Author of the following commits did not sign a Contributor Agreement:
99e68d9, a69ef7e, aa4b9a2, af93be4, c88e611, bd0af6e, 45760a8, fa8d3ab, 0020034, 0c57059, 3e452ae, 6c7c36c, 7f2bb57, 7f86139, 8525ac9, 608537b

Please, read and sign the above mentioned agreement if you want to contribute to this project

@@ -0,0 +1,12 @@
py -m pip install -U pytest
Copy link
Contributor

Choose a reason for hiding this comment

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

This list of install calls can be replaced by py -m pip install .\tests\requirements\requirements-base.txt I believe

@@ -0,0 +1,10 @@
call .\tests\scripts\install_chocolatey.bat
Copy link
Member

Choose a reason for hiding this comment

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

Will it be possible to add some docs/headers about how this particular script works, what arguments are required and what requirements are needed?

@@ -0,0 +1,6 @@
mkdir .\tests\tempFeatures
Copy link
Member

Choose a reason for hiding this comment

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

Will it be possible to add some docs/headers about how this particular script works, what arguments are required and what requirements are needed?

@@ -0,0 +1,6 @@
mkdir .\tests\tempFeatures
curl https://codeload.github.com/elastic/apm/zip/master -o .\tests\tempFeatures\features.zip
Copy link
Member

Choose a reason for hiding this comment

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

is codeload.github.com the official and supported URL?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the URL they use when you click on the download button on the website. We use it for a long time already and never had problems (other than the occasional network glitch)

Copy link
Member

Choose a reason for hiding this comment

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

I was not familiar with that particular entrypoint, then I searched and found that's not officially supported but some internal entrypoint which it could potentially be changed. The reference I found is below one:

We could potentially use curl -L https://github.com/elastic/apm/archive/master.zip to fetch the artifact, similar to what we do in the apm-integration-testing, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, although I don't anticipate them changing those URLs anytime soon, they are mentioned in the docs, even if only as en example response: https://developer.github.com/v3/repos/contents/#get-archive-link

Copy link
Member

Choose a reason for hiding this comment

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

It's not clear what they do support or not! Anyway, no strong opinion, up to @ElWPenn how to proceed then. I'm glad to know a bit more about this! Thanks Beni for the feedback

@@ -0,0 +1 @@
@"%SystemRoot%\System32\WindowsPowerShell\v1.0\powershell.exe" -NoProfile -InputFormat None -ExecutionPolicy Bypass -Command " [System.Net.ServicePointManager]::SecurityProtocol = 3072; iex ((New-Object System.Net.WebClient).DownloadString('https://chocolatey.org/install.ps1'))" && SET "PATH=%PATH%;%ALLUSERSPROFILE%\chocolatey\bin" No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

Is this required? IIUC, the choco binary is available for the Windows CI Workers

@@ -0,0 +1,12 @@
%1 -m pip install -U pytest
Copy link
Member

Choose a reason for hiding this comment

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

Will it be possible to add some docs/headers about how this particular script works, what arguments are required and what requirements are needed?

@@ -0,0 +1,8 @@
SETLOCAL EnableDelayedExpansion
Copy link
Member

Choose a reason for hiding this comment

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

Will it be possible to add some docs/headers about how this particular script works, what arguments are required and what requirements are needed?

@beniwohli beniwohli mentioned this pull request Mar 4, 2020
4 tasks
@v1v
Copy link
Member

v1v commented Mar 10, 2020

This PR has been superseded with #746

@v1v v1v closed this Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants