-
Notifications
You must be signed in to change notification settings - Fork 61
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
refactor build_deploy.sh #893
refactor build_deploy.sh #893
Conversation
fbb75d2
to
bc3b963
Compare
/retest |
1 similar comment
/retest |
e9c91a9
to
2ebe86d
Compare
/retest |
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 looks good to me! You'll notice I didn't put any comments in the main script changes. That's because I ran it locally and it worked fine. I looked at it line by line and it all seems sane and readable. I will say I had to ask chatgpt to tell me what these two lines did
[[ 1 -eq $(jq '.tags | length' <<<"$response") ]]
! git diff --quiet "$target_branch" -- "${BASE_IMAGE_FILES[@]}"
If you felt moved to throw a comment above each those lines I think it would help readability a bit. But great patch thanks for doing this!
Hey Adam, thanks for the review. On the first one, I didn't write that line, and it already had a comment, which I left intact. it reads...
The second one, I've added a comment but just because you asked nicely :P ... sorry, now joking aside, I was hoping the function name would provide enough context. But I added the comment anyways, the function looks like this now:
Hope that helps ! |
d5d0121
to
60747e5
Compare
This changes provides the following:
push
commands if running locally (outside of a CI job)