Skip to content

Conversation

@bharathappali
Copy link
Collaborator

Earlier the path was incorrect and as a result the jenkins job log was missing the summary table. The file is now created at the root dir (i.e openjdk-docker) and will be updated by the build_image function to log the docker build status. In the end the table is displayed and the temporary file .summary_table is deleted.

@dinogun Can i please have it reviewed ? Thanks in advance.

Signed-off-by: bharathappali [email protected]

@dinogun
Copy link
Collaborator

dinogun commented Jan 21, 2021

Looking at the summary_table related functions, I think thee is no need for them to be in common_functions.sh as it is only used in build_latest.sh. I think it would be better to move all of the functions to build_latest.sh, then you wont need to recompute summary_table_file in multiple places.

@bharathappali
Copy link
Collaborator Author

@dinogun Thanks for your review. Earlier I have added summary table related functions in common_functions.sh file thinking it can be used anywhere in build_latest or build_all but you were right it makes sense to add them in build_all.sh where they are being used in the current implementation. I have made the changes accordingly. Can i please have the latest changes reviewed ?

build_all.sh Outdated
}

function remove_summary_table_file() {
rm -rf ${summary_table_file}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest rm -f as it is just a file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aah! I shouldn't have added a recursive option. Changed it to rm -f

build_all.sh Outdated
source ./common_functions.sh

# summary table array
summary_table_file="./.summary_table"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can define it here and export it so that you dont have to redefine in build_latest.sh. In build_latest.sh you can check if it is not set and set it only if needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made changes accordingly. Had also exported root_dir in build_all.sh

Copy link
Collaborator

@dinogun dinogun left a comment

Choose a reason for hiding this comment

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

lgtm

@dinogun dinogun merged commit dd4ed8c into AdoptOpenJDK:master Jan 22, 2021
@karianna karianna added the bug label Jan 27, 2021
@karianna karianna added this to the January 2021 milestone Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants