-
Notifications
You must be signed in to change notification settings - Fork 6
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
License Burp Suite Pro for the vnc user #57
Conversation
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.
Good stuff! 👍
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.
🏆
As a point of order, should the checklist items in the PR that occur after merging go in the associated issue instead? We'd talked before about wanting all the boxes to be checked to consider a PR ready to review.
I remember talking about that. The problem with putting those tasks in the associated issue is that it makes it vastly easier to forget to do them - that's why I like them being in the PR. As an alternative, what if I created separate sections titled "Post-approval checklist" and "Post-merge checklist" and put those checkboxes there? As an example I went ahead and did that with this PR. Those sections won't normally be required, except in the case of these |
I am good with adding these "Post-approval" and "Post-merge" checklists where needed. We could add them to the template and leave them blank with a comment asking the PR creator to add those items if necessary. |
We should probably rename "Checklist" to "Pre-approval checklist" in that case. I did this above as an example. |
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.
Glad you found the solution ✔
…sting" This reverts commit 576e2da. The branch in question has been merged into develop via cisagov/ansible-role-burp-suite-pro#5.
🗣 Description
This pull request makes the change of licensing Burp Suite Pro for the
vnc
user.💭 Motivation and Context
See cisagov/ansible-role-burp-suite-pro#5.
🧪 Testing
I built a staging AMI with this code and verified on env0 of our staging COOL environment that Burp Suite Pro is now pre-licensed.
✅ Pre-approval checklist
✅ Post-approval checklist
develop
branch of cisagov/ansible-role-burp-suite-pro once Allow per-user licensing ansible-role-burp-suite-pro#5 is merged.✅ Post-merge checklist