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.

Sign up for GitHub

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

Jump to bottom

Resolve issue 23 #32

Merged
merged 6 commits into from
Jun 26, 2019
Merged

Resolve issue 23 #32

merged 6 commits into from
Jun 26, 2019

Conversation

mcdonnnj
Copy link
Member

Add support for cleaning up an AWS bucket used to store files so only X newest files are retained per #23 .

@mcdonnnj mcdonnnj requested review from felddy, jsf9k and dav3r June 25, 2019 21:45
@mcdonnnj mcdonnnj self-assigned this Jun 25, 2019
jsf9k
jsf9k requested changes Jun 26, 2019
View reviewed changes
Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

This looks very good to me. Nice work sir!

I had one comment that needs to be addressed, either by making a change or explaining why it isn't possible. After that is resolved I will approve.

aws_jobs/cyhy-data-extract.py Outdated Show resolved Hide resolved
dav3r
dav3r requested changes Jun 26, 2019
View reviewed changes
Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

Nice work here @mcdonnnj, but I do have a couple of comments (one more important than the other) that I'd like to see addressed before I approve this one.

aws_jobs/cyhy-data-extract.py Outdated Show resolved Hide resolved
aws_jobs/cyhy-data-extract.py Outdated Show resolved Hide resolved
@mcdonnnj mcdonnnj requested review from jsf9k and dav3r June 26, 2019 14:35
@mcdonnnj mcdonnnj mentioned this pull request Jun 26, 2019
Merged
jsf9k
jsf9k approved these changes Jun 26, 2019
View reviewed changes
Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

Approved! Strong work.

dav3r
dav3r approved these changes Jun 26, 2019
View reviewed changes
Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

Excellente! 🎆

@mcdonnnj mcdonnnj merged commit f873b57 into develop Jun 26, 2019
@mcdonnnj mcdonnnj deleted the resolve_issue_23 branch June 26, 2019 23:09
@mcdonnnj mcdonnnj mentioned this pull request Jun 27, 2019
Closed
@mcdonnnj mcdonnnj mentioned this pull request Dec 6, 2019
Merged
9 tasks
mcdonnnj added a commit that referenced this pull request Mar 26, 2020
@mcdonnnj
Merge pull request #32 from cisagov/improvement/add_python_requires
945a733 
Add python_requires argument for setup.py
mcdonnnj added a commit that referenced this pull request Mar 26, 2020
@mcdonnnj
Merge pull request #32 from cisagov/improvements/update_python_versio…
8116a89 
…n_and_checkout_action

Update Python and actions/checkout Versions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsf9k jsf9k jsf9k approved these changes

@dav3r dav3r dav3r approved these changes

@felddy felddy Awaiting requested review from felddy felddy is a code owner

Assignees

@mcdonnnj mcdonnnj

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mcdonnnj @jsf9k @dav3r