niflows / niflow-manager

Niflow package manager tool
Other
2 stars 8 forks source link

adding functions for test, build and install (python only) #26

Closed djarecka closed 4 years ago

djarecka commented 4 years ago
pep8speaks commented 4 years ago

Hello @djarecka! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 3:16: E401 multiple imports on one line Line 11:80: E501 line too long (82 > 79 characters) Line 12:80: E501 line too long (81 > 79 characters) Line 28:80: E501 line too long (81 > 79 characters) Line 68:80: E501 line too long (94 > 79 characters) Line 73:80: E501 line too long (87 > 79 characters)

Line 10:80: E501 line too long (87 > 79 characters)

Line 23:80: E501 line too long (93 > 79 characters)

Line 23:80: E501 line too long (91 > 79 characters) Line 27:80: E501 line too long (87 > 79 characters)

Comment last updated at 2020-01-05 19:56:56 UTC
codecov[bot] commented 4 years ago

Codecov Report

Merging #26 into master will decrease coverage by 9.71%. The diff coverage is 78.51%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #26      +/-   ##
=======================================
- Coverage   95.72%   86%   -9.72%     
=======================================
  Files           5     8       +3     
  Lines         117   243     +126     
  Branches       20    40      +20     
=======================================
+ Hits          112   209      +97     
- Misses          2    16      +14     
- Partials        3    18      +15
Flag Coverage Δ
#unittests 86% <78.51%> (-9.72%) :arrow_down:
Impacted Files Coverage Δ
niflow_manager/cli/main.py 100% <100%> (ø) :arrow_up:
niflow_manager/cli/install.py 71.42% <71.42%> (ø)
niflow_manager/cli/build.py 73.4% <73.4%> (ø)
niflow_manager/cli/test.py 92.85% <92.85%> (ø)

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 4550034...50862ab. Read the comment docs.

djarecka commented 4 years ago

@effigies , @satra - could you please take a look

djarecka commented 4 years ago

@satra @effigies - I was just debugging "weird bug", really couldn't understand why the tests suddenly stoped working and the reason was that I updated my branch on github and this branch is used in the container to install nfm that I need to install the package (using nfm install /nfm that for python gives pip install /nfm/package): https://github.com/niflows/niflow-manager/pull/26/files#diff-df7d959148596f5488c0d2b2d8be5fa2R67

Sure, in the future, it won't use my branch, but the issue remains - it's possible that running nfm build on my laptop I can use nfm install from different versions of nfm-manager. Any idea how to do it better? in a way that it works even if someone does not have the niflow-manager repository? Or maybe it's fine as it is now?

satra commented 4 years ago

a version check between the spec and the nfm manager. i.e. when released it should always work on a given version of the spec (like docker, docker-compose).

djarecka commented 4 years ago

@satra - the issue has nothing to do with the spec.

In order to build the container that has the package installed:

My local niflow-manager might be different than the one in the container. Usually it shouldn't be a problem, I was having issue because I was changing install method itself. Perhaps, it's fine, but just something to keep in mind

djarecka commented 4 years ago

please let me know if you have any suggestions that have to be addressed in this pr

satra commented 4 years ago

@djarecka - at this point i'm ok with merging this.

djarecka commented 4 years ago

@satra - I've added the entrypoint to the build part (change also the key from the requirements), but don't really use it in the tests, we should think about it more at some point

The current spec description is not enough, but would prefer to improve it in a separate pr

djarecka commented 4 years ago

@satra , @effigies - you can se my last changes suggested by Satra, but if you think that this can be merge, someone should click the merge button, I don't have write permission here