-
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
Tools transition prepare release #91
Changes from 96 commits
082974d
e332ab5
831ca88
27f2c88
5fcd72b
2eae7db
ffeeaa4
6c7332f
02f4452
271e0d4
6f2f692
0d5079a
09fcf54
eddb078
e8318ad
a98f448
28ffcd1
9189081
b34d6e5
cb83901
a325539
476ab31
72605a5
f353723
7e86b5d
db00b6a
03f4e8f
69f06f0
9cf2e12
6fa30fe
eaa0484
094699d
25d7ebb
b35c6ef
b4078c6
3c95b3d
365dede
ec33dba
a737746
0d7ba17
fecb105
eb6ea07
394fae4
3e039cf
d9e267c
4fa9f9d
ad2bffc
fb4199e
3855dd2
0b47a81
ef50082
46c07f2
4aa84b4
9bff43c
b015bf6
872ff5c
22fc213
8a706d0
39606ba
e55710c
8698ee1
1bb9bea
b4d1761
b68a5bc
c13b72c
a2e8aef
85dc231
acf19f7
3aa3f98
a956c2a
1330096
0b9b606
ffb1142
c40558f
217ccf2
8461511
1273fe2
89c0e63
b89e7eb
14159a4
551fade
b869041
48ff45c
d75a8cf
e62e3bc
a85e2f2
34b2514
7e72c1f
6860248
901ea18
fa1005a
9193cf6
cd390b6
addc33e
7307718
634c931
5e1e907
9c1c198
ae37809
080f34a
616f253
48768d9
ee70e54
3ba99ef
bfffcb8
3aac329
81a4413
db1b32e
5fbb9bc
1d4ec86
2abe570
f91c2dc
25ff7c1
64aec37
c53aa9c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,9 @@ name: Tool Tests | |
|
||
env: | ||
GH_TOKEN: ${{ secrets.GH_TOKEN }} | ||
MICROSOFT_EMAIL: [email protected] | ||
USER_NAME: Gurkan Indibay | ||
MAIN_BRANCH: all-citus | ||
|
||
on: | ||
push: | ||
|
@@ -25,10 +28,14 @@ jobs: | |
steps: | ||
- name: Checkout repository | ||
uses: actions/checkout@v2 | ||
- name: Define git credentials | ||
run: git config --global user.email "${MICROSOFT_EMAIL}"&& git config --global user.name "${USER_NAME}" | ||
- name: Install package dependencies | ||
run: sudo apt install libcurl4-openssl-dev libssl-dev | ||
- name: Install python requirements | ||
run: python -m pip install -r python/requirements.txt | ||
- name: Execute unit tests | ||
run: python -m pytest -q python/tests/test_update_package_properties.py | ||
run: python -m pip install -r packaging_automation/requirements.txt | ||
- name: Execute unit tests for "Update Package Properties" | ||
run: python -m pytest -q packaging_automation/tests/test_update_package_properties.py | ||
- name: Execute unit tests for "Prepare Release" | ||
run: python -m pytest -q packaging_automation/tests/test_prepare_release.py | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,233 @@ | ||||||
import re | ||||||
import subprocess | ||||||
from datetime import datetime | ||||||
from typing import Dict, List | ||||||
import os | ||||||
|
||||||
from github import Repository, PullRequest, Commit | ||||||
from jinja2 import Environment, FileSystemLoader | ||||||
|
||||||
from .common_validations import (is_tag, is_version) | ||||||
from git import Repo | ||||||
import pathlib2 | ||||||
from typing import Tuple | ||||||
|
||||||
BASE_GIT_PATH = pathlib2.Path(__file__).parents[1] | ||||||
PATCH_VERSION_MATCH_FROM_MINOR_SUFFIX = "\.\d{1,3}" | ||||||
|
||||||
|
||||||
def get_spec_file_name(project_name: str) -> str: | ||||||
return f"{project_name}.spec" | ||||||
|
||||||
|
||||||
def get_project_version_from_tag_name(tag_name: is_tag(str)) -> str: | ||||||
return tag_name[1:] | ||||||
|
||||||
|
||||||
def get_template_environment(template_dir: str) -> Environment: | ||||||
file_loader = FileSystemLoader(template_dir) | ||||||
env = Environment(loader=file_loader) | ||||||
return env | ||||||
|
||||||
|
||||||
def find_nth_occurrence_position(subject_string: str, search_string: str, n) -> int: | ||||||
start = subject_string.find(search_string) | ||||||
|
||||||
while start >= 0 and n > 1: | ||||||
start = subject_string.find(search_string, start + 1) | ||||||
n -= 1 | ||||||
return start | ||||||
|
||||||
|
||||||
def find_nth_matching_line_and_line_number(subject_string: str, regex_pattern: str, n: int) -> Tuple[int, str]: | ||||||
"""Takes a subject string, regex param and the search index as parameter and returns line number of found match. | ||||||
If not found returns -1""" | ||||||
lines = subject_string.splitlines() | ||||||
counter = 0 | ||||||
for line_number, line in enumerate(lines): | ||||||
if re.match(regex_pattern, line): | ||||||
counter = counter + 1 | ||||||
if counter == n: | ||||||
return line_number, lines[line_number] | ||||||
return -1, "" | ||||||
|
||||||
|
||||||
def remove_text_with_parenthesis(param: str) -> str: | ||||||
"""Removes texts within parenthesis i.e. outside parenthesis(inside parenthesis)-> outside parenthesis """ | ||||||
return re.sub(r"[(\[].*?[)\]]", "", param) | ||||||
onurctirtir marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
|
||||||
def run(command, *args, **kwargs): | ||||||
result = subprocess.run(command, *args, check=True, shell=True, **kwargs) | ||||||
return result | ||||||
|
||||||
|
||||||
def cherry_pick_prs(prs: List[PullRequest.PullRequest]): | ||||||
for pr in prs: | ||||||
commits = pr.get_commits() | ||||||
for single_commit in commits: | ||||||
if not is_merge_commit(single_commit): | ||||||
cp_result = run(f"git cherry-pick {single_commit.commit.sha}") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We usually use
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||||||
print( | ||||||
f"Cherry pick result for PR no {pr.number} and commit sha {single_commit.commit.sha}: {cp_result}") | ||||||
|
||||||
|
||||||
def get_minor_version(version: str) -> str: | ||||||
project_version_details = get_version_details(version) | ||||||
return f'{project_version_details["major"]}.{project_version_details["minor"]}' | ||||||
|
||||||
|
||||||
def get_patch_version_regex(version: is_version(str)): | ||||||
return fr"^{re.escape(get_minor_version(version))}{PATCH_VERSION_MATCH_FROM_MINOR_SUFFIX}$" | ||||||
|
||||||
|
||||||
def is_merge_commit(commit: Commit): | ||||||
return len(commit.parents) <= 1 | ||||||
|
||||||
|
||||||
def get_version_details(version: is_version(str)) -> Dict[str, str]: | ||||||
version_parts = version.split(".") | ||||||
return {"major": version_parts[0], "minor": version_parts[1], "patch": version_parts[2]} | ||||||
|
||||||
|
||||||
def get_upcoming_patch_version(version: is_version(str)) -> str: | ||||||
project_version_details = get_version_details(version) | ||||||
return f'{get_upcoming_minor_version(version)}.{int(project_version_details["patch"]) + 1}' | ||||||
|
||||||
|
||||||
def get_upcoming_minor_version(version: is_version(str)) -> str: | ||||||
upcoming_version_details = get_version_details(version) | ||||||
return f'{upcoming_version_details["major"]}.{upcoming_version_details["minor"]}' | ||||||
|
||||||
|
||||||
def is_major_release(version: is_version(str)) -> bool: | ||||||
version_info = get_version_details(version) | ||||||
return version_info["patch"] == "0" | ||||||
|
||||||
|
||||||
def str_array_to_str(str_array: List[str]) -> str: | ||||||
return f"{os.linesep.join(str_array)}{os.linesep}" | ||||||
|
||||||
|
||||||
def get_prs_for_patch_release(repo: Repository.Repository, earliest_date: datetime, base_branch: str, | ||||||
last_date: datetime = None): | ||||||
pull_requests = repo.get_pulls(state="closed", base=base_branch, sort="created", direction="desc") | ||||||
|
||||||
# filter pull requests according to given time interval | ||||||
filtered_pull_requests = list() | ||||||
for pull_request in pull_requests: | ||||||
if not pull_request.is_merged(): | ||||||
continue | ||||||
if pull_request.merged_at < earliest_date: | ||||||
continue | ||||||
if last_date and pull_request.merged_at > last_date: | ||||||
continue | ||||||
|
||||||
filtered_pull_requests.append(pull_request) | ||||||
|
||||||
# finally, sort the pr's by their merge date | ||||||
sorted_pull_requests = sorted(filtered_pull_requests, key=lambda p: p.merged_at) | ||||||
return sorted_pull_requests | ||||||
|
||||||
|
||||||
def filter_prs_by_label(prs: List[PullRequest.PullRequest], label_name: str): | ||||||
filtered_prs = [] | ||||||
for pr in prs: | ||||||
if any(label.name == label_name for label in pr.labels): | ||||||
filtered_prs.append(pr) | ||||||
return filtered_prs | ||||||
|
||||||
|
||||||
def file_includes_line(base_path: str, relative_file_path: str, line_content: str) -> bool: | ||||||
with open(f"{base_path}/{relative_file_path}", "r") as reader: | ||||||
content = reader.read() | ||||||
lines = content.splitlines() | ||||||
for line in lines: | ||||||
if line == line_content: | ||||||
return True | ||||||
return False | ||||||
|
||||||
|
||||||
def count_line_in_file(base_path: str, relative_file_path: str, search_line: str) -> int: | ||||||
with open(f"{base_path}/{relative_file_path}", "r") as reader: | ||||||
content = reader.read() | ||||||
lines = content.splitlines() | ||||||
return len(list(filter(lambda line: line == search_line, lines))) | ||||||
|
||||||
|
||||||
def replace_line_in_file(file: str, match_regex: str, replace_str: str) -> bool: | ||||||
with open(file, "r") as reader: | ||||||
file_content = reader.read() | ||||||
lines = file_content.splitlines() | ||||||
has_match = False | ||||||
for line_number, line in enumerate(lines): | ||||||
if re.match(match_regex, line.strip()): | ||||||
has_match = True | ||||||
lines[line_number] = replace_str | ||||||
edited_content = str_array_to_str(lines) | ||||||
with open(file, "w") as writer: | ||||||
writer.write(edited_content) | ||||||
|
||||||
return has_match | ||||||
|
||||||
|
||||||
def append_line_in_file(file: str, match_regex: str, append_str: str) -> bool: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I couldn't quite understand what we are exactly doing in Can you improve the readability of those two functions, or at least let's comment why do we do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||||||
with open(file, "r+") as reader: | ||||||
file_content = reader.read() | ||||||
lines = file_content.splitlines() | ||||||
has_match = False | ||||||
copy_lines = lines.copy() | ||||||
appended_line_index = 0 | ||||||
for line_number, line in enumerate(lines): | ||||||
if re.match(match_regex, line.strip()): | ||||||
has_match = True | ||||||
|
||||||
if line_number + 1 < len(lines): | ||||||
copy_lines[appended_line_index + 1] = append_str | ||||||
lines_to_be_shifted = lines[line_number + 1:] | ||||||
copy_lines = copy_lines[0:appended_line_index + 2] + lines_to_be_shifted | ||||||
else: | ||||||
copy_lines.append(append_str) | ||||||
appended_line_index = appended_line_index + 1 | ||||||
edited_content = str_array_to_str(copy_lines) | ||||||
with open(file, "w") as writer: | ||||||
writer.write(edited_content) | ||||||
|
||||||
return has_match | ||||||
|
||||||
|
||||||
def prepend_line_in_file(file: str, match_regex: str, append_str: str) -> bool: | ||||||
with open(file, "r+") as reader: | ||||||
file_content = reader.read() | ||||||
lines = file_content.splitlines() | ||||||
has_match = False | ||||||
copy_lines = lines.copy() | ||||||
prepended_line_index = 0 | ||||||
for line_number, line in enumerate(lines): | ||||||
if re.match(match_regex, line.strip()): | ||||||
has_match = True | ||||||
copy_lines[prepended_line_index] = append_str | ||||||
lines_to_be_shifted = lines[line_number:] | ||||||
copy_lines = copy_lines[0:prepended_line_index + 1] + lines_to_be_shifted | ||||||
prepended_line_index = prepended_line_index + 1 | ||||||
edited_content = str_array_to_str(copy_lines) | ||||||
with open(file, "w") as writer: | ||||||
writer.write(edited_content) | ||||||
|
||||||
return has_match | ||||||
|
||||||
|
||||||
def get_current_branch(working_dir: str) -> str: | ||||||
repo = Repo(working_dir) | ||||||
return repo.active_branch | ||||||
|
||||||
|
||||||
def does_branch_exist(branch_name: str) -> bool: | ||||||
repo = Repo(BASE_GIT_PATH) | ||||||
return branch_name in repo.references | ||||||
|
||||||
|
||||||
def get_template_environment(template_dir: str) -> Environment: | ||||||
file_loader = FileSystemLoader(template_dir) | ||||||
env = Environment(loader=file_loader) | ||||||
return env |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import string_utils | ||
from parameters_validation import parameter_validation | ||
import re | ||
|
||
CITUS_MINOR_VERSION_PATTERN = r"\d{1,2}\.\d{1,2}" | ||
CITUS_PATCH_VERSION_PATTERN = CITUS_MINOR_VERSION_PATTERN + r"\.\d{1,2}" | ||
|
||
|
||
@parameter_validation | ||
def is_version(version: str): | ||
if not version: | ||
raise ValueError("version should be non-empty and should not be None") | ||
if not re.match(CITUS_PATCH_VERSION_PATTERN, version): | ||
raise ValueError( | ||
"version should include three level of digits separated by dots, e.g: 10.0.1") | ||
|
||
|
||
@parameter_validation | ||
def is_tag(tag: str): | ||
if not tag: | ||
raise ValueError("tag should be non-empty and should not be None") | ||
if not re.match(f"v{CITUS_PATCH_VERSION_PATTERN}", tag): | ||
raise ValueError( | ||
"tag should start with 'v' and should include three level of digits separated by dots, e.g: v10.0.1") | ||
|
||
|
||
@parameter_validation | ||
def is_email(email: str): | ||
if not string_utils.is_email(email): | ||
raise ValueError("Parameter is not in email format") |
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.
Sharing some additional comments from my manual testing:
Minor release flow seems pretty fine.
Major release flow:
Release branch creation works fine.
But the changes made for master branch doesn't seem to be correct. Please check with the commit that I shared before
Getting some errors after running the script for the second time:
For the missing/incorrect parameters, I would expect throwing errors beforehand, but for the most, we throw the error in very deeper levels (like when using github api or when splitting a string etc.).
Let's make some correctness checks for them at the very beginning of our main entrypoint.
I think we can have a function to do those checks such as
check_args
or something.In
initialize_env
function, we should decide which repo to clone according toprj_name
arg.It can be either "citus" or "citus-enterprise". So I think we can use
add_arg(..choices=..)
for that.For
--is_test
&cherry_pick_enabled
params, I think we can useadd_arg(..action='store_true'..
, instead of requiring to pass "True" string.For
schema_version
, I think we should error if it's passed for major release.But we should be okay if it is passed or not passed for patch release.
I think we should set
add_arg(..required=true
..), forprj_ver
,main_branch
&gh_token
.We should error if either of
cherry_pick_enabled
orearliest_pr_date
is set for major release.But we should be okay if it is passed or not passed for patch release.
We should error if
cherry_pick_enabled
is set to true butearliest_pr_date
is not given.Can we simplify
earliest_pr_date
format, like just'%Y.%m.%d
? I don't think we need to know hour, minute etc. ever.Seems that we currently don't use
exec_path
arg, can we remove that ?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.
I addressed all of them except the action="store_true" parameters because when this parameter is used, it can not be set by user. It sets a constant variable not suitable for user-defined parameters
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.
I don't actually think it cannot be set by user.
Assuming
is_test
arg, usingstore_true
means:If user doesn't add
--is_test
, thenis_test
would automatically evaluate tofalse
, so we will disable test mode.If user puts
--is_test
into the args (without any additional strings), thenargparese
should interpretis_test
astrue
.https://stackoverflow.com/a/8203679/13370386
Is there any reason for not using that ?
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.
I hadn't totally grasped the parameter. As you referenced, I understood and changed that way thanks 👍