-
Notifications
You must be signed in to change notification settings - Fork 15
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
GCE: Add 'tf-network' to the list of protected networks #328
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #328 +/- ##
==========================================
+ Coverage 79.11% 79.12% +0.02%
==========================================
Files 22 22
Lines 1656 1657 +1
==========================================
+ Hits 1310 1311 +1
Misses 346 346 ☔ View full report in Codecov by Sentry. |
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.
LGTM
b906aaf
to
7726a6d
Compare
ocw/lib/gce.py
Outdated
@@ -11,7 +12,11 @@ | |||
|
|||
class GCE(Provider): | |||
__instances = {} | |||
__skip_networks = frozenset({"default"}) | |||
|
|||
if PCWConfig.has('cleanup/gce-skip-networks'): |
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.
__skip_networks = frozenset(ConfigFile().getList('cleanup/gce-skip-networks', "default"))
- will replace whole if/else block
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.
No. The .getList()
does not have 'default value'.
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.
https://github.com/SUSE/pcw/blob/master/webui/PCWConfig.py#L47
def getList(self, config_path: str, default: list = None) -> list:
if default is None:
default = []
return [i.strip() for i in self.get(config_path, ','.join(default)).split(',')]
and what about second parameter named default
?
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.
so what do you suggest me to do?
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.
replace
if PCWConfig.has('cleanup/gce-skip-networks'):
__skip_networks = frozenset(ConfigFile().getList('cleanup/gce-skip-networks'))
else:
__skip_networks = frozenset({"default"})
with one-liner __skip_networks = frozenset(ConfigFile().getList('cleanup/gce-skip-networks', "default"))
unless I am missing something your 4 lines and mine one-liner suppose to do exactly same thing 🤔
012297e
to
386a95c
Compare
In GCE the networking and security resources don't have tags neither metadata. Also the delete protection feature isn't available for those. Instead, all of those resources have link to the 'network' resource, which we have whitelist for. So I: 1) Make this whitelist of GCE `skip-networks` configurable in `pcw.ini` 2) Add `tf-network` to this list (see https://gitlab.suse.de/qac/terraform)
386a95c
to
775b7be
Compare
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.
LGTM
In GCE the networking and security resources don't have tags neither metadata.
Also the delete protection feature isn't available for those. Instead,
all of those resources have link to the 'network' resource, which we
have whitelist for. So I:
skip-networks
configurable inpcw.ini
tf-network
to this list (see https://gitlab.suse.de/qac/terraform)