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

Add audit script to get list of commits from Apache Spark master branch #3791

Merged
merged 4 commits into from
Oct 15, 2021

Conversation

nartal1
Copy link
Collaborator

@nartal1 nartal1 commented Oct 11, 2021

This PR is to add script to get the list of commits to be audited for Spark-3.3(master) branch.
We already have similar script to get list of commits for Spark-3.2(branch-3.2). The scripts are separated so that it would be easier to setup Jenkins job to get the commits for different branches.

@nartal1 nartal1 added this to the Oct 4 - Oct 15 milestone Oct 11, 2021
@nartal1 nartal1 self-assigned this Oct 11, 2021
@nartal1
Copy link
Collaborator Author

nartal1 commented Oct 11, 2021

build

@razajafri
Copy link
Collaborator

Most of the file is a copy of the file scripts/audit-spark-3.2.sh. Can we modularize it?

@nartal1
Copy link
Collaborator Author

nartal1 commented Oct 12, 2021

Most of the file is a copy of the file scripts/audit-spark-3.2.sh. Can we modularize it?

I had kept it separate as it would be easier to setup jenkins job for separate Spark versions(as we have different commit ids) and also it would be easier to remove the scripts/jenkins job once we stop supporting a particular version. Please let me know if it's okay.

@nartal1
Copy link
Collaborator Author

nartal1 commented Oct 12, 2021

I will update the current script so that it reflects commits from different Spark version.

Signed-off-by: Niranjan Artal <[email protected]>
@nartal1
Copy link
Collaborator Author

nartal1 commented Oct 12, 2021

build

@sameerz sameerz added the tools label Oct 13, 2021
scripts/audit-spark.sh Outdated Show resolved Hide resolved
Signed-off-by: Niranjan Artal <[email protected]>
@nartal1
Copy link
Collaborator Author

nartal1 commented Oct 13, 2021

build

@nartal1
Copy link
Collaborator Author

nartal1 commented Oct 14, 2021

@razajafri @tgravescs I think I have addressed the review comments. PTAL. Have verified this script against the jenkins job and it works fine.

Copy link
Collaborator

@razajafri razajafri left a comment

Choose a reason for hiding this comment

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

I still see some hard coded values like the SHA1 of commits, shouldn't they be parameterized as well?

git checkout $tag
git log --oneline HEAD...990bee9c58e -- sql/core/src/main sql/catalyst/src/main > b3.1.1.log
git log --oneline HEAD...79a6e00b7621bb -- sql/core/src/main sql/catalyst/src/main > previousVersion.log
Copy link
Collaborator

Choose a reason for hiding this comment

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

79a6e00b7621bb should this be parameterized as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the recent common hash between 2 git branches. Have parameterized this as well now. PTAL.

Comment on lines +33 to +34
basebranch="master"
tag="branch-3.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

same for these, can/should they be parameterized?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are default values. basebranch and tag are parameterized.

tgravescs
tgravescs previously approved these changes Oct 14, 2021
Signed-off-by: Niranjan Artal <[email protected]>
@nartal1
Copy link
Collaborator Author

nartal1 commented Oct 14, 2021

build

@nartal1 nartal1 merged commit b492263 into NVIDIA:branch-21.12 Oct 15, 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.

4 participants