Tools transition prepare release #91

gurkanindibay · 2021-04-09T09:32:22Z

No description provided.

…into tools-transition

Move changelog into test dir

….9 features but GithubActions environment is 3.8.5

…ameters from pipeline parameters

Add checks to prevent Add checks and test cases to prevent multiple changelog add

…into tools-transition

Add trigger to run on all branches and pr requests

onurctirtir · 2021-06-02T12:59:08Z

.github/workflows/tool-tests.yml

@@ -2,6 +2,9 @@ name: Tool Tests



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:

.... .... subprocess.CalledProcessError: Command 'git checkout -b release-10.2-test' returned non-zero exit status 128.

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 to prj_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 use add_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..), for prj_ver, main_branch & gh_token.

We should error if either of cherry_pick_enabled or earliest_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 but earliest_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 ?

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

I don't actually think it cannot be set by user.
Assuming is_test arg, using store_true means:
If user doesn't add --is_test, then is_test would automatically evaluate to false, so we will disable test mode.
If user puts --is_test into the args (without any additional strings), then argparese should interpret is_test as true.

https://stackoverflow.com/a/8203679/13370386

Is there any reason for not using that ?

I hadn't totally grasped the parameter. As you referenced, I understood and changed that way thanks 👍

* Fix cloned directory deletion error * Add repo parameter to support citus-enterprise * Add default value for main branch * Use some argparse features to improve UX * Add some more validations

creation

release

resources

* Add comment for resource_to_be_deleted array

onurctirtir

Works fine for community releases, but there are some problems with enterprise releases.
Please fix those problems by comparing the example commits that I shared via Teams chat.

Also, please add a good readme on how to use this script. Sharing an example in below, let's use a nicer wording.
Also please use your reasoning to make our README user-friendly. For example, you can consider a scenario where someone other than me or Hanefi does the prepare release stage:

(file name is something like README-prepare-release / README-prepare-release.txt)

First make sure that you have the dependencies for our script:
< one liner instruction to install dependencies >

For both major & patch release, provide `--prj-name` either as "citus" or "citus-enterprise".
Also provide `--main_branch` as `master` or `enterprise-master` depending on --prj-name` param.

For major release, do:
python3.x something --some params -- other params --an example version

For patch release, do:
python3.x something --some params -- other params --an example version

If you would like to bump the schema version for the patch release, also add X param like:
python3.x something --some params -- other params --an example version

If you would like to cherry pick commits for a patch release, also add Y, X .. params like:
python3.x something --some params -- other params --an example version

packaging_automation/common_tool_methods.py

onurctirtir · 2021-06-10T09:03:21Z

packaging_automation/common_tool_methods.py

+    return has_match
+
+
+def append_line_in_file(file: str, match_regex: str, append_str: str) -> bool:


I couldn't quite understand what we are exactly doing in append_line_in_file & prepend_line_in_file.

Can you improve the readability of those two functions, or at least let's comment why do we do anIndex +1 / anIndex +2 kind of things.

packaging_automation/common_tool_methods.py

onurctirtir · 2021-06-10T09:19:52Z

packaging_automation/common_tool_methods.py

+        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}")


We usually use -x parameter to cherry-pick the commits:

-x When recording the commit, append a line that says "(cherry picked from commit ...)" to the original commit message in order to indicate which commit this change was cherry-picked from. ...

Suggested change

cp_result = run(f"git cherry-pick {single_commit.commit.sha}")

cp_result = run(f"git cherry-pick -x {single_commit.commit.sha}")

functions * Remove does prefixes from branch check functions * Fix comments on repo references

onurctirtir

Looks pretty much fine to me, thank you for all the work & addressing the comments.
Please follow squash strategy to merge your pr.

Left some minor final notes:

I run the script a few times and only in one of them it asked for sudo, not in the others.
Might worth to make sure that we fixed that issue.
Seeing three files like test1e58b070-b0bb-4f78-9ae4-3672bd131097, do we ever use them (and why) ? If not, can we remove them ?
For patch releases, we create two (release) test branches, can we avoid creating the later one (the one with uuid suffix) ?:

  release-10.0-test
  release-10.0-test_da502a7a-109e-44d5-92c6-d042e095b7e0

onurctirtir · 2021-06-11T08:09:41Z

packaging_automation/README.md

+```
+#### Patch
+``` console
+python -m  packaging_automation.prepare_release --gh_token <your-personal-token> --prj_name citus-enterprise --prj_ver 10.0.4 --schema_version 10.0-5


let's also add here --main-branch enterprise-master

There is no need to add main-branch because main branch is set by project_name if not given as parameter

tools/packaging_automation/prepare_release.py

Line 563 in 25ff7c1

main_branch = arguments.main_branch if arguments.main_branch else repo_details[arguments.prj_name]["branch"]

packaging_automation/README.md

gurkanindibay · 2021-06-11T10:02:34Z

Looks pretty much fine to me, thank you for all the work & addressing the comments.
Please follow squash strategy to merge your pr.

Left some minor final notes:

I run the script a few times and only in one of them it asked for sudo, not in the others.
Might worth to make sure that we fixed that issue.

Seeing three files like test1e58b070-b0bb-4f78-9ae4-3672bd131097, do we ever use them (and why) ? If not, can we remove them ?

For patch releases, we create two (release) test branches, can we avoid creating the later one (the one with uuid suffix) ?:
  release-10.0-test
  release-10.0-test_da502a7a-109e-44d5-92c6-d042e095b7e0

I haven't experiences such an issue. I opened an issue about it. prepare release sometimes needs sudo #106
Fixed
The latter one with uuid is the one being used for PR. The other one with test is the release branch for test. If test is not used this branch will not be created. This branch is for testing purposes not to break release branches

gurkanindibay and others added 30 commits March 31, 2021 18:00

Initial test pipeline

082974d

Fix path problem

e332ab5

Add update_package_properties and its unit tests

831ca88

Remove github token from scripts

27f2c88

Fix requirements.txt path

5fcd72b

Remove unattended upgrades since gives error

2eae7db

Remove unneccessary pkgvars file

ffeeaa4

Merge branch 'tools-transition' of https://github.com/citusdata/tools …

6c7332f

…into tools-transition

Update tool-tests.yml

02f4452

Update requirements path

271e0d4

Update test path

6f2f692

Add missing requirements

0d5079a

Remove missing package from requirements

09fcf54

Fix Paths for test

eddb078

Move changelog into test dir

Install python 3.9

e8318ad

Install python 3.9

a98f448

Change removeprefix and removesuffix since this features are python 3…

28ffcd1

….9 features but GithubActions environment is 3.8.5

Add validation for GITHUB_TOKEN

9189081

Fix validation for GITHUB_TOKEN

b34d6e5

Add parameter GH_TOKEN into pipeline yml

cb83901

Remove logs and fix name for the last task

a325539

Fix missing parameter problems on update_al_changes

476ab31

Fix bash command

72605a5

Add update package properties

f353723

Add flags to make script fail when a step fail

7e86b5d

remove parameter set on update_package_properties.sh to get these par…

db00b6a

…ameters from pipeline parameters

Fix some static code warnings

03f4e8f

Add checks to prevent Add checks and test cases to prevent multiple changelog add

Merge branch 'tools-transition' of https://github.com/citusdata/tools …

69f06f0

…into tools-transition

Remove unnecessary files

9cf2e12

Change pipeline name

6fa30fe

Add trigger to run on all branches and pr requests

gurkanindibay added 2 commits June 2, 2021 13:29

* Fix some bugs in corner cases

7307718

* Remove unnecessary logs

634c931

onurctirtir reviewed Jun 2, 2021

View reviewed changes

gurkanindibay added 2 commits June 3, 2021 11:25

Fix round #4 review notes from @onur

5e1e907

* Fix cloned directory deletion error * Add repo parameter to support citus-enterprise * Add default value for main branch * Use some argparse features to improve UX * Add some more validations

Fix unit test error on test release branch

9c1c198

creation

gurkanindibay requested a review from onurctirtir June 3, 2021 08:44

gurkanindibay added 7 commits June 3, 2021 13:34

Use store_true argument for boolean values

ae37809

Remove unnecessary log

080f34a

Add remote and local branch checks

616f253

Fix unit test for local branch check

48768d9

Fix unit test for remote branch check

ee70e54

Add remote and local branch checks on prepare

3ba99ef

release

Use citus as test source

bfffcb8

hanefi self-requested a review June 8, 2021 15:10

gurkanindibay added 3 commits June 9, 2021 06:26

Fix version update error on major execution

3aac329

Fix unit test errors while teardown test

81a4413

resources

* Remove unnecessary imports

db1b32e

* Add comment for resource_to_be_deleted array

onurctirtir reviewed Jun 10, 2021

View reviewed changes

gurkanindibay added 4 commits June 10, 2021 15:23

* Add comment on append line and prepend line

5fbb9bc

functions * Remove does prefixes from branch check functions * Fix comments on repo references

Fix configure.in project name for citus enterprise

1d4ec86

Add readme for prepare-release script

2abe570

Remove sudo from rm statement

f91c2dc

gurkanindibay requested a review from onurctirtir June 10, 2021 15:33

Remove does prefix from does_branch_exist

25ff7c1

onurctirtir approved these changes Jun 11, 2021

View reviewed changes

gurkanindibay added 2 commits June 11, 2021 12:55

Fix parameter names in README

64aec37

Remove unnecessary files

c53aa9c

gurkanindibay merged commit 213855e into develop Jun 11, 2021

onurctirtir mentioned this pull request Jun 17, 2021

prepare_release.pl is broken for citus xy.z.t since it uses hardcoded digits to parse citus version #86

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tools transition prepare release #91

Tools transition prepare release #91

gurkanindibay commented Apr 9, 2021

onurctirtir Jun 2, 2021 •

edited

Loading

gurkanindibay Jun 3, 2021

onurctirtir Jun 3, 2021 •

edited

Loading

gurkanindibay Jun 3, 2021

onurctirtir left a comment •

edited

Loading

onurctirtir Jun 10, 2021

gurkanindibay Jun 10, 2021

onurctirtir Jun 10, 2021

gurkanindibay Jun 10, 2021

onurctirtir left a comment

onurctirtir Jun 11, 2021

gurkanindibay Jun 11, 2021

gurkanindibay commented Jun 11, 2021

		return has_match


		def append_line_in_file(file: str, match_regex: str, append_str: str) -> bool:

	cp_result = run(f"git cherry-pick {single_commit.commit.sha}")
	cp_result = run(f"git cherry-pick -x {single_commit.commit.sha}")

Tools transition prepare release #91

Tools transition prepare release #91

Conversation

gurkanindibay commented Apr 9, 2021

onurctirtir Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurctirtir Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurctirtir left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurctirtir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gurkanindibay commented Jun 11, 2021

onurctirtir Jun 2, 2021 •

edited

Loading

onurctirtir Jun 3, 2021 •

edited

Loading

onurctirtir left a comment •

edited

Loading