Skip to content
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

update-dockerfile-update-workflow #19147

Merged
merged 36 commits into from
Aug 20, 2023
Merged

Conversation

sapirshuker
Copy link
Contributor

Status

In Progress

Related Content Pull Request

Related PR: link to the PR at demisto/content

Related Issues

Related: https://jira-hq.paloaltonetworks.local/browse/CIAC-7666

Description

migration of the Python version will trigger a GitHub workflow to open PRs and update the docker image, (update_dokcer_file in GitHub -> workflows -> update_internal/external_base_images utils/auto_dockerfile_update/update_dockerfiles.py)
in this workflow, we will add a mechanism to compare the base image in the docker file and the image tag in the pipfile/pyproject.tomal file, If we have an inequality we will update the pipfile and
run poetry-lock --no-update or pipfile lock --keep-outdated to update the python version in the pipfile.lock/poetry.lock file - this will keep the repo updated.

@JudahSchwartz JudahSchwartz requested review from eyalpalo and removed request for eyalpalo August 14, 2023 06:32
utils/auto_dockerfile_update/update_dockerfiles.py Outdated Show resolved Hide resolved
utils/auto_dockerfile_update/get_dockerfiles.py Outdated Show resolved Hide resolved
echo "Approving and merging"
gh pr review --approve "$PR_URL"
gh pr merge --auto --squash "$PR_URL"
# - name: approve and merge
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reminder to revert

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert

@@ -22,7 +23,7 @@ jobs:
GITHUB_TOKEN: ${{secrets.CONTENT_BOT_TOKEN}}
run: |
git fetch
branches=$(git branch -r | grep autoupdate/)
branches=$(git branch -r | grep TESTautoTESTupdate/)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reminder to revert

@@ -1,10 +1,13 @@
name: Update external base images
name: Update external base images test
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert

utils/auto_dockerfile_update/get_dockerfiles.py Outdated Show resolved Hide resolved
utils/auto_dockerfile_update/update_dockerfiles.py Outdated Show resolved Hide resolved
except Exception as e:
print(f"{e}: Can't read file {file_path}")
if python_version:
full_str_python_version = python_version.group(0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whats the difference? Why do we need both of these

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The full_str_python_version is "python_version= "3.9"" format, we need it in order to replace it in the pipfile/pyproject file.
The string_version_number is only the version 3.9

utils/auto_dockerfile_update/update_dockerfiles.py Outdated Show resolved Hide resolved
utils/auto_dockerfile_update/update_dockerfiles.py Outdated Show resolved Hide resolved
@sapirshuker sapirshuker marked this pull request as ready for review August 16, 2023 08:59
Copy link
Contributor

@eyalpalo eyalpalo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! We should revert all the 'Test' changes you made to check this step.

.github/workflows/create-update-dockerfiles-PR.yml Outdated Show resolved Hide resolved
.github/workflows/create-update-dockerfiles-PR.yml Outdated Show resolved Hide resolved
.github/workflows/create-update-dockerfiles-PR.yml Outdated Show resolved Hide resolved
echo "Approving and merging"
gh pr review --approve "$PR_URL"
gh pr merge --auto --squash "$PR_URL"
# - name: approve and merge
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert

.github/workflows/update-external-base-images.yml Outdated Show resolved Hide resolved
utils/auto_dockerfile_update/update_dockerfiles.py Outdated Show resolved Hide resolved
utils/auto_dockerfile_update/update_dockerfiles.py Outdated Show resolved Hide resolved
utils/auto_dockerfile_update/update_dockerfiles.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@JudahSchwartz JudahSchwartz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great job!
a few minor comments
please run ruff on the file

utils/auto_dockerfile_update/update_dockerfiles.py Outdated Show resolved Hide resolved
utils/auto_dockerfile_update/update_dockerfiles.py Outdated Show resolved Hide resolved
utils/auto_dockerfile_update/update_dockerfiles.py Outdated Show resolved Hide resolved
utils/auto_dockerfile_update/update_dockerfiles.py Outdated Show resolved Hide resolved
utils/auto_dockerfile_update/update_dockerfiles.py Outdated Show resolved Hide resolved
utils/auto_dockerfile_update/update_dockerfiles.py Outdated Show resolved Hide resolved
None
"""
update_result = False
image_name = dockerfile['image_name']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is the case we will only update python. we want to update every image based on python as well

Copy link
Collaborator

@JudahSchwartz JudahSchwartz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! A few last minor changes

return False


def change_python_version(file_path: str, str_version: str) -> Tuple[bool, str]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fn doesnt seem to do anything except log. Lets log that in the inner function and delete this 1

utils/auto_dockerfile_update/update_dockerfiles.py Outdated Show resolved Hide resolved
if completed_process.returncode != 0:
os.chdir(current_directory)
return False
except subprocess.CalledProcessError as e:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

collapese these catch blocks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need this block in order to get the stderr from the CalledProcessError (It creates a object with the data)

utils/auto_dockerfile_update/update_dockerfiles.py Outdated Show resolved Hide resolved
@sapirshuker sapirshuker requested review from eyalpalo and removed request for eyalpalo August 20, 2023 08:08
@JudahSchwartz JudahSchwartz dismissed eyalpalo’s stale review August 20, 2023 08:10

dismissing since Eyal is on vacation

@sapirshuker sapirshuker merged commit 269cc2b into master Aug 20, 2023
3 checks passed
@welcome
Copy link

welcome bot commented Aug 20, 2023

Congrats on merging your first pull request! 🎉 How awesome!

@sapirshuker sapirshuker deleted the update-dockerfile-update-workflow branch August 20, 2023 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants