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

Doctl support for database firewalls management #950

Merged
merged 7 commits into from
Feb 10, 2021
Merged

Doctl support for database firewalls management #950

merged 7 commits into from
Feb 10, 2021

Conversation

danaelhe
Copy link
Member

@danaelhe danaelhe commented Feb 2, 2021

adding the following functionality:
doctl databases firewalls list
doctl databases firewalls add --rule type:value
doctl databases firewalls remove --rule type:value
doctl databases firewalls update --rule type:value [--rule type:value]

@danaelhe
Doctl support for database firewalls management
7955b13 
adding the following functionality:
doctl databases firewalls list <db uuid>
doctl databases firewalls add <db uuid> --rule type:value
doctl databases firewalls remove <db uuid> --rule type:value
doctl databases firewalls update <db uuid> --rule type:value [--rule type:value]
@danaelhe danaelhe requested a review from a team February 2, 2021 13:47
@danaelhe danaelhe requested review from a team and removed request for a team February 2, 2021 14:13
@andrewsomething andrewsomething linked an issue Feb 2, 2021 that may be closed by this pull request
Support Trusted Sources in Managed Database? #711
Closed
bentranter
bentranter requested changes Feb 2, 2021
View reviewed changes
Copy link
Member

@bentranter bentranter left a comment

Choose a reason for hiding this comment

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

Looks great overall!

A few minor details about the tests and how to add the godo changes.

integration/database_firewall_add_test.go Outdated Show resolved Hide resolved
integration/database_firewall_add_test.go Outdated Show resolved Hide resolved
vendor/github.com/digitalocean/godo/databases.go Outdated Show resolved Hide resolved
ChiefMateStarbuck
ChiefMateStarbuck previously requested changes Feb 2, 2021
View reviewed changes
commands/databases.go Outdated Show resolved Hide resolved
commands/databases.go Show resolved Hide resolved
commands/databases.go Outdated Show resolved Hide resolved
commands/databases.go Outdated Show resolved Hide resolved
commands/databases.go Outdated Show resolved Hide resolved
commands/databases.go Show resolved Hide resolved
commands/databases.go Show resolved Hide resolved
bentranter and others added 3 commits February 8, 2021 12:03
@bentranter
Update mock server implementation
c57b3e9
@danaelhe
Resolving test issues
874cf09 
fix test issues for add and remove.
@danaelhe
removing created at value in displayer
3a617fc 
created at is enforced at database level and cannot be
overriden.
@danaelhe
Make code work without changing godo
09abca3 
adjusted the code to work without the need to change anything in godo.
@danaelhe danaelhe requested a review from a team February 9, 2021 18:42
ChiefMateStarbuck
ChiefMateStarbuck previously approved these changes Feb 9, 2021
View reviewed changes
Copy link
Contributor

@ChiefMateStarbuck ChiefMateStarbuck left a comment

Choose a reason for hiding this comment

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

LGTM

andrewsomething
andrewsomething requested changes Feb 9, 2021
View reviewed changes
Copy link
Member

@andrewsomething andrewsomething left a comment

Choose a reason for hiding this comment

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

Looking great! A couple tiny suggestions in-line.

commands/databases.go Outdated Show resolved Hide resolved
do/databases.go Outdated Show resolved Hide resolved
do/databases.go Outdated Show resolved Hide resolved
commands/databases.go Outdated Show resolved Hide resolved
andrewsomething
andrewsomething approved these changes Feb 9, 2021
View reviewed changes
Copy link
Member

@andrewsomething andrewsomething left a comment

Choose a reason for hiding this comment

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

👍 Looks good to merge on green!

@danaelhe danaelhe removed the request for review from bentranter February 10, 2021 15:39
danaelhe
danaelhe commented Feb 10, 2021
View reviewed changes
Copy link
Member Author

@danaelhe danaelhe left a comment

Choose a reason for hiding this comment

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

all changes have been applied

danaelhe
danaelhe commented Feb 10, 2021
View reviewed changes
Copy link
Member Author

@danaelhe danaelhe left a comment

Choose a reason for hiding this comment

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

viewed.

bentranter
bentranter approved these changes Feb 10, 2021
View reviewed changes
Copy link
Member

@bentranter bentranter left a comment

Choose a reason for hiding this comment

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

LGTM!

@danaelhe danaelhe merged commit 9b40e1a into main Feb 10, 2021
@tlhunter tlhunter mentioned this pull request Aug 17, 2021
Closed
@ChiefMateStarbuck ChiefMateStarbuck deleted the db-firewall-rules branch March 7, 2022 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bentranter bentranter bentranter approved these changes

@andrewsomething andrewsomething andrewsomething approved these changes

@ChiefMateStarbuck ChiefMateStarbuck ChiefMateStarbuck left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Trusted Sources in Managed Database?
4 participants
@danaelhe @andrewsomething @bentranter @ChiefMateStarbuck