-
Notifications
You must be signed in to change notification settings - Fork 21
[CLOUDP-350567] Automate PR creation to release helm chart using gh helm repo #514
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
base: master
Are you sure you want to change the base?
Conversation
MCK 1.5.0 Release NotesNew Features
Bug Fixes
|
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.
can we move script to release/helm_charts
subdir?
if not github_token: | ||
logger.info("Warning: GH_TOKEN environment variable not set.") | ||
pr_url = f"https://github.com/{REPO_NAME}/pull/new/{branch_name}" | ||
logger.info(f"Please create the Pull Request manually by following the link:\n{pr_url}") |
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.
after github_token is not found it should return
env: | ||
GH_TOKEN: ${GH_TOKEN} | ||
MCK_DIR: ${workdir}/src/github.com/mongodb/mongodb-kubernetes | ||
binary: scripts/dev/run_python.sh scripts/create_chart_release_pr.py --chart_version ${RELEASE_OPERATOR_VERSION|*triggered_by_git_tag} |
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.
RELEASE_OPERATOR_VERSION
is not there anymore, please use OPERATOR_VERSION
instead
args = parser.parse_args() | ||
|
||
chart_version = args.chart_version | ||
branch_name = f"mck-release-{chart_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.
q: what happens if the previous mck-release-{chart_version}
branch was merged and we want to make a fix to the same helm chart version? Will this work?
chart_version = args.chart_version | ||
branch_name = f"mck-release-{chart_version}" | ||
|
||
workdir = os.environ.get("MCK_DIR") |
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.
do we actually need this specified? Cannot we just call pathlib.Path().resolve()
?
If we run the script locally, it working dir should be proper one. If we run the script on evg host we just need to set working_dir: src/github.com/mongodb/mongodb-kubernetes
and it will also work.
run_command(["git", "add", "."], cwd=helm_repo_path) | ||
run_command(["git", "commit", "-m", commit_message], cwd=helm_repo_path) | ||
|
||
if github_token: |
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 would check for github_token
in the beginning and later just assume it is set
|
||
run_command(["git", "push", "-u", "origin", branch_name], cwd=helm_repo_path) | ||
|
||
create_pull_request(branch_name, chart_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.
If create_pull_request
returns 0 or 1 in case of errors we should propagate the result to main. Currently the result is swallowed.
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.
Code generally looks good, but I have two general comments:
- instead of logging error and returning the error code, it's easier to just repackage the error and raise it again:
except GithubException as e:
raise Exception(f"ERROR: Could not create Pull Request. GitHub API returned an error: {e.status}, details: {e.data}, original error: {e}")
- the main method does a little too much. It should only handle flags parsing and validation. After that call nested method with validated args that does the hard work.
Summary
Per current process when we try to release a version of the helm chart we create a PR to the
mongodb/helm-charts
repo. Since this is manual task, it's time consuming and error prone.This PR tries to automate that manual task.
Proof of Work
The successful run here created the PR on helm chart repo successfully.
Checklist
skip-changelog
label if not needed