Dynamic patch update #2469
Dynamic patch update #2469
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #2469 +/- ##
==========================================
- Coverage 88.71% 88.57% -0.14%
==========================================
Files 180 180
Lines 14227 14103 -124
==========================================
- Hits 12621 12492 -129
- Misses 1606 1611 +5
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## dev #2469 +/- ##
==========================================
- Coverage 88.71% 88.57% -0.14%
==========================================
Files 180 180
Lines 14227 14103 -124
==========================================
- Hits 12621 12492 -129
- Misses 1606 1611 +5
|
|
@blueandgold I don't know if creating 2 remote branches to facilitate a newly created test in https://docs.google.com/spreadsheets/d/117QLTjAbvzfrdnhFWgIh22A-M0MiK2YvUwU7EJgTQag/edit#gid=1172914269 is feasible. When a new release comes out for this feature to be tested forseti must be installed with that new releases code. Hence creating a static branch 88.0.0 to install from and see if 88.0.1 is what gets installed on vm won't work, because 88.0.0 will always be using the same code rather than the new release's |
install/gcp/installer/util/utils.py
Outdated
| end_const = segments[2][2:] | ||
| else: | ||
| end_const = segments[2][1:] | ||
| return "%s.{[0-9],[0-9][0-9]}%s" % (start_const, end_const) |
There was a problem hiding this comment.
I don't get why end_const is needed. Also, why we are slicing parts from segment[2].
The example that you showed me was:
*88.0.{[0-9], [0-9][0-9]}*
I must have missed something, but I can't retrieve anything with how it's used.
git tag -l *88.0.{[0-9], [0-9][0-9]}*
install/gcp/installer/util/utils.py
Outdated
| @@ -106,6 +106,37 @@ def _print_banner(border_symbol, edge_symbol, corner_symbol, | |||
| print(border) | |||
| print('') | |||
|
|
|||
| def get_latest_patch_query(): | |||
There was a problem hiding this comment.
The naming of this method can be better, to reflect what it is doing. I suggest:
def get_all_patches_glob():
def get_all_patches_glob_in_current_version():
def get_glob_for_all_patches_in_current_version():
#1: you are finding all patches, not just the latest patch
#2: glob is more clear than query, it matches the documentation and when we discuss verbally
| @@ -91,6 +91,7 @@ def get_deployment_values(self): | |||
| 'BUCKET_LOCATION': self.config.bucket_location, | |||
| 'GCP_CLIENT_SERVICE_ACCOUNT': self.gcp_service_acct_email, | |||
| 'FORSETI_VERSION': self.version, | |||
| 'MATCHING_PATCHES_QUERY': self.matching_patches_query, | |||
There was a problem hiding this comment.
The naming here would be more readable as:
GLOB_FOR_ALL_PATCHES_IN_CURRENT_VERSION
install/gcp/installer/util/utils.py
Outdated
| end_const = segments[2][2:] | ||
| else: | ||
| end_const = segments[2][1:] | ||
| return "%s.{[0-9],[0-9][0-9]}%s" % (start_const, end_const) |
There was a problem hiding this comment.
We should write a unit test for this method. All the return results should be verified.
deployment-templates/compute-engine/server/forseti-instance-server.py
Outdated
Show resolved
Hide resolved
| def glob_expr_matching_patches(forseti_version): | ||
| #TODO update in server/forseti-instance-server if update here | ||
| """Returns a glob expression matching all patches to the version of passed in parameter""" | ||
| segments = forseti_version[6:].split('.') |
There was a problem hiding this comment.
Instead of slicing this by [6:], how about doing a string replace of tags/v, which would be more readable.
| #TODO update in server/forseti-instance-server if update here | ||
| """Returns a glob expression matching all patches to the version of passed in parameter""" | ||
| segments = forseti_version[6:].split('.') | ||
| if forseti_version[:6] == "tags/v" and all(segment.isdigit() for segment in segments): |
There was a problem hiding this comment.
You can decompose this for better readability and debugging.
if forseti_version[:6] != 'tags/v':
return None
for segment in segments:
if not segment.isdigit():
return None
return "v{}.{}.{{[0-9],[0-9][0-9]}}".format(segments[0], segments[1])
| #TODO update in server/forseti-instance-server if update here | ||
| """Returns a glob expression matching all patches to the version of passed in parameter""" | ||
| segments = forseti_version[6:].split('.') | ||
| if forseti_version[:6] == "tags/v" and all(segment.isdigit() for segment in segments): |
There was a problem hiding this comment.
It seems nicer to move the tags/v check to the beginning of the method.
| """Returns a glob expression matching all patches to the version of passed in parameter""" | ||
| segments = forseti_version[6:].split('.') | ||
| if forseti_version[:6] == "tags/v" and all(segment.isdigit() for segment in segments): | ||
| return "v{}.{}.{{[0-9],[0-9][0-9]}}".format(segments[0], segments[1]) |
There was a problem hiding this comment.
We use only single-quotes, not double-quotes. Please change everywhere.
| FORSETI_VERSION = ( | ||
| "git checkout {forseti_version}".format( | ||
| forseti_version=context.properties['forseti-version'])) | ||
| patch_search_expression = glob_expr_matching_patches(context.properties['forseti-version']) |
There was a problem hiding this comment.
For better readability, you can write like this:
patch_search_expression = get_patch_search_expression(context.properties['forseti-version'])
deployment-templates/compute-engine/client/forseti-instance-client.py
Outdated
Show resolved
Hide resolved
| @@ -150,8 +178,8 @@ def GenerateConfig(context): | |||
| # Install Forseti. | |||
| download_forseti=DOWNLOAD_FORSETI, | |||
|
|
|||
| # Checkout Forseti version. | |||
| checkout_forseti_version=FORSETI_VERSION, | |||
| # if on tag, checkout latest patch to that version, else checkout current branch version | |||
There was a problem hiding this comment.
Please use proper sentence, capitalize first letter and give it a period. Also keep it to 80 character length. You can set the margin in pycharm.
deployment-templates/compute-engine/client/forseti-instance-client.py
Outdated
Show resolved
Hide resolved
| @@ -14,6 +14,15 @@ | |||
|
|
|||
| """Creates a GCE instance template for Forseti Security.""" | |||
|
|
|||
| def glob_expr_matching_patches(forseti_version): | |||
| #TODO update in server/forseti-instance-server if update here | |||
| """Returns a glob expression matching all patches to the version of passed in parameter""" | |||
There was a problem hiding this comment.
Document the arg and the return.
| patch=${{segments[2]}} | ||
| patch=${{patch: 0: 2}} | ||
| patch=$(echo $patch | sed 's/[^0-9]*//g') | ||
| if !((${{#latest_version[@]}})) || ((patch > ${{latest_version[1]}})); |
There was a problem hiding this comment.
What does this bash code do in this line?
There was a problem hiding this comment.
Sets latest version to the version currently being iterated if latest version hasnt yet been instantiated or if the version being iterated on is greater than latest version. Essentially
if latest_version is None or patch > latest_version[1]
where latest_version is an array [full_version, patch_number]
There was a problem hiding this comment.
Ah, I was thrown off by latest_version is an array [full_version, patch_number]. Can you please make a comment to make this easier to read? Or else, rename this to something more clear.
# latest_version is an array [full_version, patch_number]
| def get_patch_search_expression(forseti_version): | ||
| """Returns a glob expression matching all patches to the version of passed in parameter | ||
|
|
||
| #TODO update in server/forseti-instance-server if update here |
There was a problem hiding this comment.
Inside the doc comment, we don't need # again. And make it a proper sentence.
TODO: Update in server/forseti-instance-server if update here.
|
|
||
| Args: | ||
| forseti_version (str): Installed forseti version. Should start with | ||
| 'tags/v' if patches are to be updated automatically |
There was a problem hiding this comment.
Line continuation has hanging indent of 4 spaces. Add period at the end.
forseti_version (str): Installed forseti version. Should start with
'tags/v' if patches are to be updated automatically.
| patch_search_expression = glob_expr_matching_patches(context.properties['forseti-version']) | ||
| if patch_search_expression: | ||
| CHECKOUT_FORSETI_VERSION = ( | ||
| """matches=$(git tag -l {patch_search_expression}) |
There was a problem hiding this comment.
Looking for change here like this:
"""versions=$(git tag -l {patch_search_expression})
| CHECKOUT_FORSETI_VERSION = ( | ||
| """matches=$(git tag -l {patch_search_expression}) | ||
| matches=(${{matches//;/ }}) | ||
| for match in "${{matches[@]}}" |
There was a problem hiding this comment.
If you make the change above, this would become
for version in "${{versions[@]}}"
| patch=${{segments[2]}} | ||
| patch=${{patch: 0: 2}} | ||
| patch=$(echo $patch | sed 's/[^0-9]*//g') | ||
| if !((${{#latest_version[@]}})) || ((patch > ${{latest_version[1]}})); |
There was a problem hiding this comment.
Ah, I was thrown off by latest_version is an array [full_version, patch_number]. Can you please make a comment to make this easier to read? Or else, rename this to something more clear.
# latest_version is an array [full_version, patch_number]
| """ | ||
| if forseti_version[:6] != 'tags/v': | ||
| return None | ||
| segments = forseti_version.replace('tags/v', '''''').split('.') |
| if forseti_version[:6] != 'tags/v': | ||
| return None | ||
| segments = forseti_version.replace('tags/v', '''''').split('.') | ||
|
|
There was a problem hiding this comment.
Please propagate the changes from above here.
deployment-templates/compute-engine/server/forseti-instance-server.py
Outdated
Show resolved
Hide resolved
| patch=${{segments[2]}} | ||
| patch=${{patch: 0: 2}} | ||
| patch=$(echo $patch | sed 's/[^0-9]*//g') | ||
| if !((${{#latest_version[@]}})) || ((patch > ${{latest_version[1]}})); |
There was a problem hiding this comment.
The comment here for latest_version would be super nice.
| fi | ||
| done | ||
| git checkout ${{latest_version[0]}}""" | ||
| # latest_version is an array [full_version, patch_number] |
There was a problem hiding this comment.
Can you document this at line 63, I think that's where this comment would really help.
There was a problem hiding this comment.
how can I put a comment in a string?
| @@ -15,6 +15,31 @@ | |||
| """Creates a GCE instance template for Forseti Security.""" | |||
|
|
|||
|
|
|||
| def get_patch_search_expression(forseti_version): | |||
| """Returns a glob expression matching all patches to the version of | |||
There was a problem hiding this comment.
The first line of a doc string needs to be one sentence within 80 characters (for generating nice-looking pydocs).
So, please change this.
"""Returns a glob expression matching all patches of the given version.
| patch=${{segments[2]}} | ||
| patch=${{patch: 0: 2}} | ||
| patch=$(echo $patch | sed 's/[^0-9]*//g') | ||
| # latest_version is an array [full_version, patch_number] |
| done | ||
| git checkout ${{latest_version[0]}}""" | ||
|
|
||
| .format(patch_search_expression=patch_search_expression)) |
There was a problem hiding this comment.
Do you need to append this .format()directly to the end of the bash string?
| @@ -124,6 +170,7 @@ def GenerateConfig(context): | |||
| # Download Forseti source code | |||
| {download_forseti} | |||
| cd forseti-security | |||
| git fetch --tags | |||
There was a problem hiding this comment.
Does git fetch --all also get all the tags?
|
|
||
| if forseti_version[:6] != 'tags/v': | ||
| return None | ||
| segments = forseti_version.replace('tags/v', '').split('.') |
Thanks for opening a Pull Request!
Here's a handy checklist to ensure your PR goes smoothly.
pylint --rcfile=pylintrcpasses.These guidelines and more can be found in our contributing guidelines.
fixes #2431
With this change, client and server vms that are installed on a version denoting tag will download and start forseti from latest patches matching that version upon vm restarts