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

Fix python 3.8 and 3.9 apache CI in dirty way #3113

Merged
merged 6 commits into from
Jan 16, 2022

Conversation

hirosassa
Copy link
Contributor

@hirosassa hirosassa commented Oct 17, 2021

Description

Related to #3091

I fixed Python 3.8 and 3.9 apache CI.
By importlib change in Python 3.9.5 release, __import__() returns absolute path of each modules or packages (this is relative path in previous version).
To maintain backward compatibility, I implemented dirty solutions (just rewrite absolute path into relative one).

I'd like to discuss my changes like this and find good solutions.

Motivation and Context

To follow Python latest versions.

Have you tested this? If so, how?

I ran test locally and GitHub Actions.

@hirosassa hirosassa requested review from dlstadther, Tarrasch and a team as code owners October 17, 2021 08:50
@hirosassa hirosassa force-pushed the fix-py38-py39-version-CI branch from 5234bab to 9b3d1c7 Compare October 17, 2021 08:57
@hirosassa hirosassa force-pushed the fix-py38-py39-version-CI branch from 9b3d1c7 to 5889c85 Compare October 17, 2021 09:05
@hirosassa hirosassa changed the title Fix python 3.8 and 3.9 apache ci in dirty way Fix python 3.8 and 3.9 apache CI in dirty way Oct 17, 2021
Copy link
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

I've really fallen behind on reviews. I see that you've been keeping this branch up-to-date. I'm good with your solutions here for the sake of stable test builds. The path manipulation isn't ideal, but it allows better coverage against github actions' python versions 3.8 and 3.9. So i'll approve and merge.

Thanks for your patience and sorry again for my massive delays.

@hirosassa
Copy link
Contributor Author

@dlstadther Thank you very much for the review!

@dlstadther
Copy link
Collaborator

Not sure what's up with the 2 tests that CI is waiting on. When i click into the Actions tab, i don't see those tests in the execution for this branch.

@hirosassa
Copy link
Contributor Author

hirosassa commented Jan 8, 2022

@dlstadther I disabled tests running on 3.8.9 and 3.9.4, and enabled 3.8 and 3.9 tests.
But the repository setting still requires 3.8.9 and 3.9.4 tests, so the tests are waiting on.
https://github.com/spotify/luigi/pull/3113/files#diff-7167d8d8ca9db60fcadbec1d7da1d3fe7fd300d6712810405f582a241d667c52L154-R156

So we need to change GitHub Actions configuration after this PR is merged.

@dlstadther
Copy link
Collaborator

Hmmm... Problem is that Github won't let me merge this PR until all tests return successful :/, including the two which are being replaced. Ideas?

@hirosassa
Copy link
Contributor Author

@dlstadther
It seems that we need to edit Require status checks to pass before merging settings in branch protection rules page (URL seems to be https://github.com/spotify/luigi/settings/branch_protection_rules/xxxxx).

@dlstadther
Copy link
Collaborator

Unfortunately, I don't have access to the repo's settings. I've only ever been a community member maintainer, never an admin or Spotify employee. So we may need one of the "dataex" members to help us out. @honnix @sonjaer @ckiosidis @jesperps

@dlstadther
Copy link
Collaborator

I looked at this a bit more and experimented, but came to the same conclusion as you @hirosassa . Without ability to modify those branch protection rules, we can't merge this :(

@hirosassa
Copy link
Contributor Author

@narape How about you Nelson?

@honnix
Copy link
Member

honnix commented Jan 14, 2022

Sorry I missed the ping. I just updated the settings.

@hirosassa
Copy link
Contributor Author

@honnix Thank you very much!

@dlstadther
Copy link
Collaborator

Thank you @honnix !

@hirosassa , feel free to update your branch with master and i'll merge once i see all greens! (no rush)

@hirosassa
Copy link
Contributor Author

@dlstadther Updated! Thank you for your help!

@dlstadther dlstadther merged commit 56b2e4f into spotify:master Jan 16, 2022
@hirosassa hirosassa deleted the fix-py38-py39-version-CI branch January 16, 2022 21:22
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.

3 participants