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

refactor build_deploy.sh #893

Merged
merged 5 commits into from
Nov 17, 2023

Conversation

Victoremepunto
Copy link
Contributor

@Victoremepunto Victoremepunto commented Nov 14, 2023

This changes provides the following:

  • Reverts multiarch support
  • Improved handling of commands evaluation
  • refactor to use CICD tools repo which provides:
    • Add temporary labels when building if PR context
    • Agnostic container engine commands
    • Automatic container registry login
    • Local runs enabled to avoid push commands if running locally (outside of a CI job)

@Victoremepunto
Copy link
Contributor Author

/retest

1 similar comment
@Victoremepunto
Copy link
Contributor Author

/retest

@adamrdrew
Copy link
Contributor

/retest

adamrdrew
adamrdrew previously approved these changes Nov 16, 2023
Copy link
Contributor

@adamrdrew adamrdrew left a 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!

Dockerfile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@Victoremepunto
Copy link
Contributor Author

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...

 33   # find all non-expired tags
 34   [[ 1 -eq $(jq '.tags | length' <<<"$response") ]]

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:

 46 _base_image_files_changed() {
 47 
 48   local target_branch=${ghprbTargetBranch:-master}
 49 
 50   # Use git to check for any non staged differences in the Base Image files
 51   ! git diff --quiet "$target_branch" -- "${BASE_IMAGE_FILES[@]}"
 52 }

Hope that helps !

@psav psav merged commit c1d5cc2 into RedHatInsights:master Nov 17, 2023
6 checks passed
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