8000 [WIP] Add [pypy] test marker for circle-ci by nixphix · Pull Request #13023 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

[WIP] Add [pypy] test marker for circle-ci #13023

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

Closed
wants to merge 1 commit into from

Conversation

nixphix
Copy link
Contributor
@nixphix nixphix commented Jan 21, 2019

Reference Issues/PRs

Fixes #13014

What does this implement/fix? Explain your changes.

Circle CI lint job will check for [pypy] marker in commit message and if available then it would trigger pypy test job.

@jnothman
Copy link
Member

It looks like circle didn't run on this pr at all. Config broken?

@nixphix
Copy link
Contributor Author
nixphix commented Jan 22, 2019

It looks like circle didn't run on this pr at all. Config broken?

I have enabled Circle CI in my fork repo, would that cause any problem?

@jnothman
Copy link
Member
jnothman commented Jan 22, 2019 via email

@nixphix
Copy link
Contributor Author
nixphix commented Jan 22, 2019

Possibly? What is its status?

It did not run, says I need to choose a plan for Mac OS to run the job, I dont know why is that necessary.

@nixphix nixphix force-pushed the ci/pypy-commit-msg-marker branch from 63bff83 to d48196e Compare January 22, 2019 16:51
@nixphix
Copy link
Contributor Author
nixphix commented Jan 22, 2019

After I disabled Circle CI in my repo, it runs here.

Now the script needs an API key to trigger the job, can someone with CI access setup environment key CIRCLE_API_USER_TOKEN with a valid API key and also rerun the lint job.

@ogrisel
Copy link
Member
ogrisel commented Jan 24, 2019

Now the script needs an API key to trigger the job, can someone with CI access setup environment key CIRCLE_API_USER_TOKEN with a valid API key and also rerun the lint job.

We cannot pass secret credentials to CI jobs run in PRs, right? Otherwise that would be a security issue: a user could submit a PR to change the CI script to display the secret token on stdout to retrieve it.

@rth
Copy link
Member
rth commented Jan 24, 2019

Yes, looks like this might a dead end due to security concerns (cf https://circleci.com/blog/managing-secrets-when-you-have-pull-requests-from-outside-contributors/).

Hmm, I guess potentially an API Token could be used to do anything on Circle CI, not just re-trigger jobs?

At the same time, the alternative proposed in #13014 (comment) of having the PyPy job always run but not do anything unless on master or if this commit flag is detected is not great either. Since green CI would not actually mean that PyPy CI passed.

Any ideas what we could do? Maybe it's an opportunity to play with Github Actions? Since we can start that Circle CI job, just not from circle CI as that would expose the token...

@adrinjalali
Copy link
Member

we already do the "run the job but return 0" on the deploy job on circle-ci. It seems pretty simple and we can make sure that we put something in the logs that the job is not actually running.

@rth
Copy link
Member
rth commented Jan 24, 2019

we already do the "run the job but return 0" on the deploy job on circle-ci.

I find that situation not ideal, basically CI status is getting less useful. For deploy, we don't really care. Here, it would make contributors think that their code passes PyPy CI while it's actually not the case.

Though, I agree that it's probably the simplest solution in the end. I'm just wondering if there is another simple way around that.

@nixphix
Copy link
Contributor Author
nixphix commented Jan 24, 2019

Are we limited to only Circle CI for running pypy tests? how about other CI vendors?

Maybe we could run the cron job in Circle CI and on-demand runs elsewhere if they are conducive for our use case.

@nixphix
Copy link
Contributor Author
nixphix commented Jan 26, 2019

Travis CI conditional job should work fine for pypy test marker

Copy link
Member
@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Small comment:


commit_msg=$(git log --format=%B -n 1 $CIRCLE_SHA1)

if [[ "$commit_msg" =~ "[pypy]" ]]
Copy link
Member

Choose a reason for hiding this comment

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

May I suggest to use "[ci pypy]" instead to make it explicit that this is about Continuous Integration?

@amueller amueller added Needs work Needs Decision Requires decision labels Aug 6, 2019
@ogrisel
Copy link
Member
ogrisel commented Dec 15, 2020

@nixphix could you try to move this PR to azure pipelines or github actions to workaround the limitations of Circle CI? github actions in particular does support conditions expressions involving commit message (we do it for the [cd build] marker for the Wheels builder workflow under the .github folder.

BTW, I have modified the configuration of the PyPy build to use conda-forge to setup the PyPy env (instead of docker).

Base automatically changed from master to main January 22, 2021 10:50
@thomasjpfan
Copy link
Member

Closing since we now run pypy test on azure which has a [pypy] marker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a commit message marker to run pypy in PRs
7 participants
0