-
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
Adds support for onebranch changes #233
Conversation
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.
Please address comments before merging this
@@ -1,5 +1,5 @@ | |||
# | |||
# This file is autogenerated by pip-compile | |||
# This file is autogenerated by pip-compile with python 3.8 |
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.
Nit, Consider introducing a .python-version
file with the intended python version that should be used here.
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.
packaging_automation/requirements.in
Outdated
@@ -16,3 +16,4 @@ urllib3 | |||
wheel | |||
python-dotenv | |||
prospector[with_everything] | |||
setuptools==58 |
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.
missing empty line at the end
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.
Fixed
parser.add_argument('--input_files_dir', required=True) | ||
|
||
args = parser.parse_args() | ||
write_postgres_versions_into_file(args.input_files_dir,args.project_version) |
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.
missing empty line at the end.
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.
Fixed
@@ -53,7 +53,7 @@ def get_filtered_package_count(session) -> int: | |||
# Since package count for our test repo is lower than 100, we get the total package details by getting all the | |||
# packages in one call | |||
result = stat_get_request( | |||
package_list_with_pagination_request_address(PACKAGE_CLOUD_PARAMETERS, 1, 100), | |||
package_list_with_pagination_request_address(PACKAGE_CLOUD_PARAMETERS, 1, 200), |
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.
Why did we need to update this?
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.
Recently I put new packages into azure repo which exceeds 100 package . I'm using azure repo as my test repo to test the package counts. After exceed, my tests failed since some packages was outside the pagination limit.
@@ -232,13 +232,14 @@ def build_package(github_token: non_empty(non_blank(str)), | |||
docker_image_name = "packaging" if not is_test else "packaging-test" | |||
postgres_extension = "all" if postgres_version == "all" else f"pg{postgres_version}" | |||
os.environ["GITHUB_TOKEN"] = github_token | |||
os.environ["CONTAINER_BUILD_RUN_ENABLED"] = "true" |
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.
This parameter is required to execute the packaging image in OneBranch.
Currently packaging images directly execute fetch_build_deb script and exits. However, OneBranch first needs to download and initialize then execute the pipeline scripts inside the container
This parameter is a switch to use the packaging images both in github actions and OneBranch pipelines
If this parameter is not set, so in OneBranch container initialisation phase, image does not exit just waits
If this is set, then the script executes as regular.
if not os.path.exists(input_output_parameters.output_dir): | ||
os.makedirs(input_output_parameters.output_dir) | ||
|
||
output = run_with_output( | ||
f'docker run --rm -v {input_output_parameters.output_dir}:/packages -v ' | ||
f'{input_output_parameters.input_files_dir}:/buildfiles:ro -e ' | ||
f'GITHUB_TOKEN -e PACKAGE_ENCRYPTION_KEY -e UNENCRYPTED_PACKAGE ' | ||
f'GITHUB_TOKEN -e PACKAGE_ENCRYPTION_KEY -e UNENCRYPTED_PACKAGE -e CONTAINER_BUILD_RUN_ENABLED ' |
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.
Parameter is added and set true to be able to execute build logic in fetch_and_build_deb/rpm scripts
@@ -3,7 +3,7 @@ GitPython | |||
Jinja2 | |||
parameters_validation | |||
pathlib2 | |||
psycopg2 | |||
psycopg2-binary |
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.
Since out github actions main VM image is ubuntu focal, just majing this script work in ubuntu was enough
Now for OneBeanch, tools python requirements scripts shoule be executed in all supported images. In debian and centos images I got error so I needed to use binary version of psycopg2-binary
packaging_automation/requirements.in
Outdated
@@ -16,3 +16,4 @@ urllib3 | |||
wheel | |||
python-dotenv | |||
prospector[with_everything] | |||
setuptools==58 |
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 got error in debian and centos images so I needed to fix the version of setuptools
@@ -53,7 +53,7 @@ def get_filtered_package_count(session) -> int: | |||
# Since package count for our test repo is lower than 100, we get the total package details by getting all the | |||
# packages in one call | |||
result = stat_get_request( | |||
package_list_with_pagination_request_address(PACKAGE_CLOUD_PARAMETERS, 1, 100), | |||
package_list_with_pagination_request_address(PACKAGE_CLOUD_PARAMETERS, 1, 200), |
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.
Recently, I added new files into azure repository in packagecloud so this broke my tests. I increased the number for records in page to be able to get correct result
@@ -0,0 +1,15 @@ | |||
import argparse |
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.
In Github Actions, we validate package counts in citus_package.py without dependency for images. Since we can not execute citus_package, we needed to externalize validation logic for OneBranch build process. We call this function explicitly after fetch_and_build_deb/rpm script
@@ -0,0 +1,11 @@ | |||
import argparse |
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.
Created because the same reason with validate_buildo_output. We execute this script just before fetch_and_build_deb/rpm to be able to get supported postgres versions
Onebranch pipelines have different structure from both Github Actions and Azure Devops since pipeline logic executes in docker images. As you may know main execution environment for both Github Actions and Azure Devops are ubuntu focal virtual machine, not container. Therefore we can execute docker run inside our python scripts
Since OneBranch pipelines does not have VM execution feature and we can only use docker images as the main execution environment and since we can not run docker run inside another docker, we need to use our docker images one by one as the main execution environment in OneBranch pipelines.
In that case we need to execute the logic inside citus_package script, which executes docker run in it, inside OneBranch pipeline scripts explicitly. Therefore we needed two new methods for validation and getting postgres versions
Additionally, current packaging images just execute a script then exists. However, this is not suitable for OneBranch container lifecyle since image is initialized and kept open until the end of the pipeline. Therefore I added a new parameter CONTAINER_BUILD_RUN_ENABLED to be able to keep the container open in absence of this parameter, since One Branch container initialization phase
We need to perform some changes in packaging/develop as well. When checking this PR, please first check all of the PRs below
citusdata/packaging#945
https://msdata.visualstudio.com/Database%20Systems/_git/citus-packaging/pullrequest/817728