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

modern python #65

Merged
merged 21 commits into from
May 30, 2021
Merged

modern python #65

merged 21 commits into from
May 30, 2021

Conversation

chayim
Copy link
Contributor

@chayim chayim commented May 16, 2021

Modern build system for python components. This includes linters, security scanning, and parallel builds across supported versions.

Also, by fixing linter errors.

closes #70
closes #71

@chayim chayim requested a review from DvirDukhan May 16, 2021 08:29
circleci docker build and publish

build instructions
@lgtm-com
Copy link

lgtm-com bot commented May 18, 2021

This pull request fixes 3 alerts when merging 34c46b5 into 6486d6e - view on LGTM.com

fixed alerts:

  • 3 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 18, 2021

This pull request fixes 3 alerts when merging 3ca1c7e into 6486d6e - view on LGTM.com

fixed alerts:

  • 3 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 18, 2021

This pull request fixes 3 alerts when merging 044fa97 into 6486d6e - view on LGTM.com

fixed alerts:

  • 3 for Unused import

@codecov
Copy link

codecov bot commented May 19, 2021

Codecov Report

Merging #65 (3a7d3a6) into master (3a7d3a6) will not change coverage.
The diff coverage is n/a.

❗ Current head 3a7d3a6 differs from pull request most recent head ec89579. Consider uploading reports for the commit ec89579 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master      #65   +/-   ##
=======================================
  Coverage   96.90%   96.90%           
=======================================
  Files           8        8           
  Lines         776      776           
=======================================
  Hits          752      752           
  Misses         24       24           

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 3a7d3a6...ec89579. Read the comment docs.

@chayim
Copy link
Contributor Author

chayim commented May 19, 2021

@DvirDukhan Can you modify the merge requirements so that this depends on ci/circleci:build-latest rather that build?

@chayim chayim marked this pull request as ready for review May 19, 2021 06:02
@lgtm-com
Copy link

lgtm-com bot commented May 19, 2021

This pull request introduces 1 alert and fixes 3 when merging b9b31e3 into 6486d6e - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a class instantiation

fixed alerts:

  • 3 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 19, 2021

This pull request introduces 1 alert and fixes 3 when merging fbc5cda into 6486d6e - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a class instantiation

fixed alerts:

  • 3 for Unused import

removing second poetry build call, since they can be done in one
@lgtm-com
Copy link

lgtm-com bot commented May 19, 2021

This pull request introduces 1 alert and fixes 3 when merging db860f6 into 6486d6e - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a class instantiation

fixed alerts:

  • 3 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 19, 2021

This pull request introduces 1 alert and fixes 3 when merging a313a28 into 3a7d3a6 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a class instantiation

fixed alerts:

  • 3 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 19, 2021

This pull request introduces 1 alert and fixes 3 when merging 5bf1426 into 3a7d3a6 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a class instantiation

fixed alerts:

  • 3 for Unused import

@chayim chayim marked this pull request as draft May 20, 2021 06:12
@lgtm-com
Copy link

lgtm-com bot commented May 20, 2021

This pull request fixes 3 alerts when merging 0509bda into 3a7d3a6 - view on LGTM.com

fixed alerts:

  • 3 for Unused import

@chayim chayim marked this pull request as ready for review May 20, 2021 09:04
@chayim
Copy link
Contributor Author

chayim commented May 20, 2021

@DvirDukhan This is ready, we've merged in the fix. Can you (please) change the repo requirement to no longer be dependent on ci/circleci:build and instead require ci/circleci:build-latest, as part of this.

While I can slam it in - I'd like a review.

@lgtm-com
Copy link

lgtm-com bot commented May 20, 2021

This pull request fixes 3 alerts when merging 98fdac3 into 3a7d3a6 - view on LGTM.com

fixed alerts:

  • 3 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 23, 2021

This pull request fixes 3 alerts when merging 4649986 into 3a7d3a6 - view on LGTM.com

fixed alerts:

  • 3 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 23, 2021

This pull request fixes 3 alerts when merging ce54a7f into 3a7d3a6 - view on LGTM.com

fixed alerts:

  • 3 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented May 26, 2021

This pull request fixes 3 alerts when merging e335fff into 3a7d3a6 - view on LGTM.com

fixed alerts:

  • 3 for Unused import


abort_for_noci:
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this a generic name across all the clients? if not can we rename to abort_tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the generic name currently in use in all clients.

@@ -0,0 +1,20 @@
ARG OSNICK=bionic
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A future goal - I'd like to build dockers out of these, and given people a one stop shop. We are not currently using it.

@@ -1,3 +1,3 @@
from .client import Client
from .client import Client # noqa
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#noqa tells the python linter to not ignore this specific line. It sets off a linter error but is required.

@sonarcloud
Copy link

sonarcloud bot commented May 30, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@lgtm-com
Copy link

lgtm-com bot commented May 30, 2021

This pull request fixes 3 alerts when merging ec89579 into 3a7d3a6 - view on LGTM.com

fixed alerts:

  • 3 for Unused import

@chayim chayim merged commit 392803d into master May 30, 2021
@chayim chayim deleted the ck-modern-py branch May 30, 2021 10:09
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.

Pass transaction and shard hints into pipeline when set. Enable postprocess on dag
3 participants