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

Hotfix/build imports issue fix #556

Merged
merged 6 commits into from
Jan 4, 2022
Merged

Conversation

bigbitbus
Copy link
Contributor

Description

Explicitly import modules when building opta binary since dynamic imports are not detected by pyinstaller.

Safety checklist

  • [X ] This change is backwards compatible and safe to apply by existing users
  • [ X] This change will NOT lead to data loss
  • [ X] This change will NOT lead to downtime who already has an env/service setup

How has this change been tested, beside unit tests?

Manually

@codecov
Copy link

codecov bot commented Dec 23, 2021

Codecov Report

Merging #556 (d035a9e) into main (f7a73a6) will increase coverage by 0.26%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #556      +/-   ##
==========================================
+ Coverage   74.10%   74.36%   +0.26%     
==========================================
  Files         100      100              
  Lines        6479     6479              
==========================================
+ Hits         4801     4818      +17     
+ Misses       1678     1661      -17     
Flag Coverage Δ
unittests 74.36% <ø> (+0.26%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
modules/aws_k8s_service/aws_k8s_service.py 72.22% <0.00%> (-0.64%) ⬇️
opta/constants.py 100.00% <0.00%> (ø)
opta/commands/apply.py 80.22% <0.00%> (+0.11%) ⬆️
opta/utils/__init__.py 91.89% <0.00%> (+1.48%) ⬆️
opta/layer.py 82.97% <0.00%> (+4.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7a73a6...d035a9e. Read the comment docs.

opta.spec Outdated
@@ -4,7 +4,37 @@ from PyInstaller.utils.hooks import collect_data_files

block_cipher = None


himps = [
Copy link
Contributor

Choose a reason for hiding this comment

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

can we sort this list for maintenance?
also it can be broken into 3 lists for each cloud providers

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we instead have a function which generates this list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RemyDeWolf @NitinAgg great suggestions! Addressed both in new commit.

@bigbitbus bigbitbus force-pushed the hotfix/build-imports-issue-fix branch from dfd48dc to fb4cefb Compare January 3, 2022 14:45
@bigbitbus
Copy link
Contributor Author

Passing tests here - https://app.circleci.com/pipelines/github/run-x/opta/514/workflows/293fb6de-d40d-46e8-9382-b485669a5d0b

Ignore the redis test - its the expiring aws eks credentials issue hashicorp/terraform#29182

opta.spec Outdated

block_cipher = None

himps = generate_module_import_array()
# himps = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: remove these comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@bigbitbus bigbitbus merged commit c5176ab into main Jan 4, 2022
@bigbitbus bigbitbus deleted the hotfix/build-imports-issue-fix branch January 4, 2022 02:33
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