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

python: flux-security bindings #1716

Merged
merged 3 commits into from Oct 12, 2018

Conversation

Projects
None yet
5 participants
@SteVwonder
Copy link
Member

SteVwonder commented Oct 11, 2018

Enables submitting signed jobspec via python

This will be less necessary once #1715 is handled, but in the meantime, I believe it is the only way to submit jobs to flux instances configured with flux-security.

@SteVwonder SteVwonder requested a review from trws Oct 11, 2018

@SteVwonder SteVwonder force-pushed the SteVwonder:py-flux-security branch from 430ad30 to 67607b5 Oct 11, 2018

Show resolved Hide resolved configure.ac
Show resolved Hide resolved t/Makefile.am

@SteVwonder SteVwonder force-pushed the SteVwonder:py-flux-security branch from 67607b5 to fb2d948 Oct 11, 2018

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Oct 11, 2018

Hmm. It seems that the configure flags (--with-flux-security in particular) don't propagate through to the distcheck. Is that the intended behavior?

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 11, 2018

Oh, you're right (just confirmed). I hadn't thought about that before, but it does seem like it's the intended behavior, e.g. see:

https://www.gnu.org/software/automake/manual/html_node/Checking-the-Distribution.html

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Oct 11, 2018

Huh. In that case, I don't think there is any benefit to having multiple distcheck's with different configure flags.

Although, that does raise the question why the t0009-security.py test (which should only run when configured with flux-security) is running in the docker container with a make distcheck. It doesn't run on my local machine and make distcheck passes just fine. I wonder if somehow the configuration outside the dist tree is affecting the make check running within the dist tree. Guess I need to poke around in the docker container.

.travis.yml Outdated
@@ -20,6 +20,7 @@ matrix:
- CC=gcc-8
- CXX=g++-8
- ARGS="--with-flux-security --enable-caliper"
- DISTCHECK_CONFIGURE_FLAGS="--with-flux-security --enable-caliper"
- DISTCHECK=t

This comment has been minimized.

@grondo

grondo Oct 12, 2018

Contributor

BTW, by default environment variables are not passed inside the docker container unless they are explicitly passed via -e VAR to docker run in docker-run-tests.sh. Sorry, that is a bit confusing.

@SteVwonder SteVwonder force-pushed the SteVwonder:py-flux-security branch from d4a9d4d to 0371833 Oct 12, 2018

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 12, 2018

Restarted the py3.6 distcheck builder which failed at the very end with a write error: stdout

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Oct 12, 2018

Another write error: stdout. Restarted again.

SteVwonder added some commits Oct 11, 2018

python: add bindings for flux-security
    - include simple test of wrap/unwrap and security error messaging
    - test python3 with flux-security in at least one travis builder

Closes #1709

@SteVwonder SteVwonder force-pushed the SteVwonder:py-flux-security branch from 0371833 to c3b2aff Oct 12, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 12, 2018

Codecov Report

Merging #1716 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1716      +/-   ##
==========================================
- Coverage   79.34%   79.32%   -0.02%     
==========================================
  Files         184      184              
  Lines       34550    34550              
==========================================
- Hits        27413    27407       -6     
- Misses       7137     7143       +6
Impacted Files Coverage Δ
src/common/libutil/veb.c 96% <0%> (-2.86%) ⬇️
src/modules/barrier/barrier.c 76.55% <0%> (-2.07%) ⬇️
src/modules/kvs-watch/kvs-watch.c 75.13% <0%> (-0.83%) ⬇️
src/modules/kvs/kvs.c 65.81% <0%> (-0.16%) ⬇️
src/modules/connector-local/local.c 74.51% <0%> (+1.35%) ⬆️
@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Oct 12, 2018

After a review from @trws, I believe this PR is ready to go in.

@trws

trws approved these changes Oct 12, 2018

Copy link
Member

trws left a comment

Looks good to me. Why adding the interactive flag on docker? Not a blocking question or anything, just curious.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 12, 2018

Why adding the interactive flag on docker?

@SteVwonder was actually fixing a bug I introduced. The script that drives docker itself has an --interactive flag so you can run in the same docker container as would be used in travis-ci for ease of reproducing issues, and I had broken support for that earlier...

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 12, 2018

Right, sounds like this is good to go. Thanks!

@garlick garlick merged commit aa7ebac into flux-framework:master Oct 12, 2018

2 of 3 checks passed

codecov/project 79.32% (-0.02%) compared to 52b3636
Details
codecov/patch Coverage not affected when comparing 52b3636...c3b2aff
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@SteVwonder SteVwonder deleted the SteVwonder:py-flux-security branch Feb 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.