-
Notifications
You must be signed in to change notification settings - Fork 3
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
Provide initial functionality #4
Conversation
@jsf9k or @mcdonnnj - Any thoughts on adding |
Sounds good to me. I agree that it should be in the skeleton. |
I realized that we already do this elsewhere and I'm not doing anything new here, so I added the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some additional feedback based on changes that have been made (and some documentation items that I missed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for implementing that change to the file header format. I have a question and a suggestion related to that change.
@mcdonnnj - Can you please circle back to this review when you have a chance? |
@jsf9k - I re-requested your review since there has been a decent amount of change since your first review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better than ever! I had some best practices suggestions and a question for your consideration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I have some comments and suggestions, but nothing that should hold up this PR.
…job" This reverts commit b6a91d1.
Co-authored-by: Shane Frasier <jeremy.frasier@gwe.cisa.dhs.gov>
Co-authored-by: Shane Frasier <jeremy.frasier@gwe.cisa.dhs.gov>
This change aligns with the code that sets this tag in cisagov/cool-assessment-terraform (see cisagov/cool-assessment-terraform#240).
Co-authored-by: Shane Frasier <jeremy.frasier@gwe.cisa.dhs.gov>
Co-authored-by: Shane Frasier <jeremy.frasier@gwe.cisa.dhs.gov>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some feedback items skimming through this again. Also a suggestion to update the lockfiles again for the final test pre-merge.
Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After more than just a skim I have some additional feedback for your consideration (sorry).
Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
Public access to items in the S3 bucket is configured in cisagov/publish-egress-ip-terraform. Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
…ch and replace Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor items for your consideration but otherwise this LGTM.
Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
…tch our example Co-authored-by: Nick <50747025+mcdonnnj@users.noreply.github.com>
🗣 Description
This PR contains the initial functionality for this Lambda, which is to scan a specified set of AWS accounts and publish files/objects (to an S3 bucket) containing the public IP addresses of EC2 instances or Elastic IPs that have been properly tagged.
💭 Motivation and context
We need a mechanism to publish egress IP info from a subset of our AWS accounts and this Lambda will allow us to do just that.
This is part of the work for:
This PR is related to the following:
🧪 Testing
In addition to passing all of the automated pre-commit checks, this code was successfully built and tested both manually (using the "Test" tab on the AWS Lambda web console) and in conjunction with the Terraform in the
cisagov/publish-egress-ip-terraform
repository.✅ Pre-approval checklist
to reflect the changes in this PR.
✅ Pre-merge checklist
✅ Post-merge checklist