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

Add functions to clean up the cloudknot config file #250

Merged
merged 25 commits into from
Sep 11, 2020

Conversation

richford
Copy link
Member

@richford richford commented Sep 4, 2020

Resolves #8
Resolves #11

This PR introduces some functions in cloudknot.config to "prune" config file entries. In this context, "prune" means to remove any config entries that do not refer to existing resources on AWS. We will provide pruning for

  • Pars
  • Knot
  • DockerRepo
  • BatchJob
  • DockerImage

This is related to #136, but does not resolve that issue. I think we should provide a CLI in another PR. This also does not solve the problem of removing unwanted resources that ARE on AWS. I'm thinking that we could do that through an interactive CLI but I'll add more details to #136 and limit the scope of this PR to config pruning through a Python API.

@richford richford added this to the v0.5.0 milestone Sep 4, 2020
@richford richford mentioned this pull request Sep 4, 2020
@richford
Copy link
Member Author

richford commented Sep 4, 2020

I believe these CI failures will be fixed after we rebase after merging #249

@richford
Copy link
Member Author

richford commented Sep 4, 2020

Okay, now I think the CI test failures will be fixed when we rebase after #249

@richford richford changed the title WIP: Add functions to clean up the cloudknot config file Add functions to clean up the cloudknot config file Sep 5, 2020
@richford
Copy link
Member Author

richford commented Sep 5, 2020

Ready for review!

@richford richford requested a review from arokem September 5, 2020 05:09
@richford
Copy link
Member Author

richford commented Sep 7, 2020

On second thought, let's hold on this until I can implement some more tests for this stuff. That would take care of #63. But it'll be a little bit before I can get to that.

@richford
Copy link
Member Author

richford commented Sep 8, 2020

Resolves #63
This is now ready for review

@richford richford linked an issue Sep 8, 2020 that may be closed by this pull request
@arokem
Copy link
Member

arokem commented Sep 9, 2020

The code looks good to me. Do you understand what Coveralls is going through?

@richford
Copy link
Member Author

richford commented Sep 9, 2020

@arokem, I believe it's related to coverallsapp/github-action#30. The coveralls github action expects an .lcov file but pytest-cov doesn't produce that format. I just pushed a new commit that pip installs coveralls inside the github action and then uses the coveralls CLI. This should work but we won't know until the NRDG org grants access to coveralls. I also requested codecov access but you can safely ignore that request if coveralls works out for us.

If it works, this would close #239 as well.

@richford
Copy link
Member Author

richford commented Sep 9, 2020

Oh, it just worked. See e.g. https://coveralls.io/jobs/67461937
We still have to:

  • grant access to the coveralls app to let it give feedback on PRs
  • then update the badge on README.md

And then I think we're done with this PR.

@richford
Copy link
Member Author

@arokem, just a reminder that the ball is in your court on approving coveralls-app access to NRDG, which I believe can be done here: https://github.com/settings/connections/applications/ad4c01c89848a307e228.

@arokem
Copy link
Member

arokem commented Sep 11, 2020 via email

@richford
Copy link
Member Author

I think it's ready now. The coverage badge won't be updated until we merge into master. And we'll have to see how coveralls responds in the next few PRs to see how we want to tweak those settings. But as far as this PR is concerned I think we're good to go.

@arokem arokem merged commit e48b6ab into nrdg:master Sep 11, 2020
@richford richford deleted the config-prune branch September 11, 2020 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test config.py in unit tests Add clean_jobs() method to config.py Add to config.prune()
2 participants